Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

U correction not correctly applied to viewport #18875

Merged
merged 5 commits into from Feb 16, 2017

Conversation

samueljackson92
Copy link
Contributor

@samueljackson92 samueljackson92 commented Feb 15, 2017

This correctly applies the U correction to the viewport when viewing a rotation surface in the instrument view.

To test:

  • Follow issue steps to reproduce bug. Apply fix and retest to confirm it is resolved.
  • Code review

Fixes #18869.


Reviewer

Please comment on the following (full description):

Code Review
  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards? Is it well structured with small focussed classes/methods/functions?
  • Are there unit/system tests in place? Are the unit tests small and test the a class in isolation?
  • If there are changes in the release notes then do they describe the changes appropriately?
Functional Tests
  • Do changes function as described? Add comments below that describe the tests performed?

  • How do the changes handle unexpected situations, e.g. bad input?

  • Has the relevant documentation been added/updated?

  • Is user-facing documentation written in a user-friendly manner?

  • Has developer documentation been updated if required?

  • Does everything look good? Comment with the ship it emoji but don't merge. A member of @mantidproject/gatekeepers will take care of it.

This code is badly indented. While we're herewe should fix it
@samueljackson92 samueljackson92 added Component: GUI Patch Candidate Urgent issues that must be included in a patch following a release High Priority An issue or pull request that if not addressed is severe enough to postponse a release. labels Feb 15, 2017
@samueljackson92 samueljackson92 added this to the Release 3.10 milestone Feb 15, 2017
@AntonPiccardoSelg AntonPiccardoSelg self-assigned this Feb 15, 2017
// Position, relative to origin
// Mantid::Kernel::V3D pos = det->getPos() -
// m_pos;
Mantid::Kernel::V3D pos = m_instrActor->getDetPos(i) - m_pos;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samueljackson92 you should talk to @SimonHeybrock and @mantid-roman about these changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part of the code is actually unchanged. Are the detector changes already available in this part of the code? Also, as I understand it, there is a chance that we could have a patch release (with these changes). Wouldn't that mean that for the patch we would have to pull in all changes required to make the new detectorInfo work in the instrument view?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, those bits have been available for quite some time now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @AntonPiccardoSelg points out the code is really unchanged. Those changes are only to whitespace. Whitespace changes are also a separate commit so could be dropped easily.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mantid-roman has an open PR that modifies this section of code. Maybe you can undo the whitespace changes to avoid merge conflicts.

std::pair<double, double> RotationSurface::calculateViewRectOffsets() {
const auto dU = fabs(m_u_max - m_u_min);
const auto dV = fabs(m_v_max - m_v_min);
auto du = dU * 0.05;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are these hard-coded values?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, this is how the code was already, so I'm not sure.

@AntonPiccardoSelg
Copy link
Contributor

Code Review:

  • Code is mainly refactored and fine.
  • I agree with Owen: the numbers should be (maybe static const) variables with a meaningfull name here. I understand that this PR is critical and I don't think that this should hold up a merge (especially since this has actually not been changed in this PR).

Functional Testing:

Before the fix:
image

After the fix:

image

The fix seems to have worked. I applied the U correction and both detector banks were visible, therefore please :shipit:

@peterfpeterson
Copy link
Member

This needs release notes.

@NickDraper
Copy link
Contributor

Final change was rst only, cancelled build to save resources

@NickDraper NickDraper merged commit a699490 into master Feb 16, 2017
@NickDraper NickDraper deleted the 18869_u_correction_instrument_view branch February 16, 2017 13:46
@samueljackson92 samueljackson92 restored the 18869_u_correction_instrument_view branch February 21, 2017 17:48
martyngigg pushed a commit that referenced this pull request Feb 23, 2017
Refs #18869 Fix U correction view rect

(cherry picked from commit 25d21c2)

Refs #18869 Fix code formatting

This code is badly indented. While we're herewe should fix it

(cherry picked from commit 9fff85c)

Revert "Refs #18869 Fix code formatting"

This reverts commit 9fff85c.

(cherry picked from commit 0deffd9)

Refs #18869 Fix blank view with no U correction

(cherry picked from commit 9667cea)

Add patch release note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
High Priority An issue or pull request that if not addressed is severe enough to postponse a release. Patch Candidate Urgent issues that must be included in a patch following a release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

U correction makes half of WISH's detectors disappear
7 participants