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

Include Scaling step in RRO Workflow Diagram #16671

Merged
merged 3 commits into from
Jun 30, 2016

Conversation

MattKing06
Copy link
Contributor

@MattKing06 MattKing06 commented Jun 21, 2016

This is a sub-issue of #16455

To test:

  • Build ReflectometryReductionOne-v1 documentation
  • Check the workflow diagram is now complete and Scaling step is included

Fixes #16660


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.

@MattKing06 MattKing06 added Reflectometry Issues and pull requests related to reflectometry Documentation Issues and pull requests related to documentation labels Jun 21, 2016
@MattKing06 MattKing06 added this to the Release 3.8 milestone Jun 21, 2016
@raquelalvarezbanos
Copy link
Contributor

@MattKing06 I think we should mention this in the release notes, as it was pointed out by scientists.

@raquelalvarezbanos
Copy link
Contributor

raquelalvarezbanos commented Jun 22, 2016

The Scaling step is displayed correctly. But now that I look at the diagram, I think the section between OutputWorkspace and Scale is not correct. From the diagram it seems that only when MomentumTransferMin AND MomentumTransferMax AND MomentumTransferStep are given, we apply the Rebin step. But looking at the code, if the first two are not given we try to calculate them from the IvsLam workspace, and if the last one, MomentumTransferStep, is not provided, we apply CalculateResolution (see here). So the three are dealt with independently and we always apply the Rebin step. Could we update this section as part of this PR?

@MattKing06
Copy link
Contributor Author

@raquelalvarezbanos I agree, I'll see if I can fix this as part of this PR.

@MattKing06 MattKing06 closed this Jun 22, 2016
@MattKing06 MattKing06 reopened this Jun 22, 2016
@raquelalvarezbanos
Copy link
Contributor

Thanks @MattKing06 I think the important thing is to clarify how these are calculated: MomentumTransferMin and MomentumTransferMax from the equation Q = 4 \pi / \lambda * sin (\theta) and MomentumTransferStep from CalculateResolution. It is also important to clarify that the Rebin in Q is always applied.

@raquelalvarezbanos
Copy link
Contributor

The workflow diagram is increasing in size but I think that is necessary if we want to document exactly what the algorithm does. Last section of the diagram (Rebin and Scale steps) looks more clear now in my opinion, it now documents how we calculate the necessary parameters for Rebin when those are not given by the user.

:shipit:

@FedeMPouzols FedeMPouzols merged commit 48330b8 into master Jun 30, 2016
@FedeMPouzols FedeMPouzols deleted the 16660_fix_workflow_diagram_for_RRO branch June 30, 2016 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Issues and pull requests related to documentation Reflectometry Issues and pull requests related to reflectometry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants