Correct error propagation on PolarizationEfficienciesWildes algorithm #39300
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MialLewis
changed the title
MialLewis
changed the title
MialLewis
added
Reflectometry
MialLewis
force-pushed
the
37768_improve_error_calc_in_PolarizationEfficienciesWildes
branch
from
c661b09 to
5a4dabb
Compare
|
This graph shoes I/Sigma for the output of As you can see, the error is reduced relative to the signal across the board.
|
…sing derived quantities
… to ensure consistent behaviour of wildes correction
MialLewis
force-pushed
the
37768_improve_error_calc_in_PolarizationEfficienciesWildes
branch
from
89d0211 to
1482c70
Compare
adriazalvarez
requested changes
May 20, 2025
There was a problem hiding this comment.
I'm currently having a segfault when running the test code in windows, only difference wrt the code in the example is that I load the file from the data archive as i don't have it locally:
from mantid.simpleapi import *
Load(Filename='POLREF00032130', OutputWorkspace='POLREF00032130')
ConvertUnits(InputWorkspace='POLREF00032130', OutputWorkspace='POLREF00032130_y', Target='Wavelength', ConvertFromPointData=False)
Rebin(InputWorkspace='POLREF00032130_y', OutputWorkspace='POLREF00032130_y_rb', Params='0.01')
SumSpectra(InputWorkspace='POLREF00032130_y_rb', OutputWorkspace='POLREF00032130_y_rb_sum')
PolarizationEfficienciesWildes(InputNonMagWorkspace='POLREF00032130_y_rb_sum', OutputFpEfficiency='fp', OutputFaEfficiency='fa', IncludeDiagnosticOutputs=True)
Seems the segfault is happening when calling evaluate workspaces in the error propagation class.
I have added a few minor comments here and there.
...ork/Algorithms/inc/MantidAlgorithms/PolarizationCorrections/PolarizationCorrectionsHelpers.h
Show resolved
Hide resolved
...ork/Algorithms/inc/MantidAlgorithms/PolarizationCorrections/PolarizationCorrectionsHelpers.h
Show resolved
Hide resolved
| } | ||
|
|
||
| private: | ||
| Func compute_func; |
There was a problem hiding this comment.
maybe camelCase is better here?
Suggested change
| Func compute_func; | |
| Func computeFunc; |
| } | ||
| }; | ||
|
|
||
| template <size_t N, typename Func> auto make_error_propagation(Func &&func) { |
There was a problem hiding this comment.
idem previous camel case comment
| const size_t specSize = outWs->blocksize(); | ||
|
|
||
| // cppcheck-suppress unreadVariable | ||
| const bool condition = Kernel::threadSafe((*args)..., *outWs); |
There was a problem hiding this comment.
Could this be renamed differently? isThreadSafe perhaps?
| } | ||
|
|
||
| PolarizationEfficienciesWildes::FlipperWorkspaces PolarizationEfficienciesWildes::getFlipperWorkspaces(const bool mag) { | ||
| if (mag) { |
There was a problem hiding this comment.
can mag be renamed to something like isMagneticWs or similar?
| void PolarizationEfficienciesWildes::mapSpinStateWorkspaces() { | ||
| const WorkspaceGroup_sptr magWsGrp = getProperty(PropNames::INPUT_MAG_WS); | ||
| const WorkspaceGroup_sptr nonMagWsGrp = getProperty(PropNames::INPUT_NON_MAG_WS); | ||
| if (magWsGrp != nullptr) { |
There was a problem hiding this comment.
Personal preference situation, you can leave it as it is if you would prefer to be more explicit.
Suggested change
| if (magWsGrp != nullptr) { | |
| if (magWsGrp) { |
There was a problem hiding this comment.
I agree, I remember spotting this but forgot to change!
Framework/Algorithms/test/PolarizationCorrections/PolarizationCorrectionsHelpersTest.h
Show resolved
Hide resolved
| const double expectedTPMO = (2 * expectedPEfficiency) - 1; | ||
| const double expectedAEfficiency = (EXPECTED_PHI / (2 * expectedTPMO)) + 0.5; | ||
| const double expectedTAMO = (2 * expectedAEfficiency) - 1; | ||
| const std::pair<double, double> expectedPEfficiency = {0.98, 0.9899494934}; |
There was a problem hiding this comment.
Another preference type situation, so feel free to leave as is, i think here compiler can infer the type without ambiguity, but as there are many const std::pair<double,double>, perhaps it would make it a bit cleaner to use an alias?
Suggested change
| const std::pair<double, double> expectedPEfficiency = {0.98, 0.9899494934}; | |
| const std::pair expectedPEfficiency = {0.98, 0.9899494934}; |
| const double expectedTPMO = (2 * expectedPEfficiency) - 1; | ||
| const double expectedAEfficiency = 0.99; | ||
| const double expectedTAMO = (2 * expectedAEfficiency) - 1; | ||
| const std::pair<double, double> expectedPEfficiency = {0.98, 0.9899494934}; |
There was a problem hiding this comment.
The expected value from the errors comes from the autodiff class? At least with the simplest error, we should add a test to compare with results taken from another method
There was a problem hiding this comment.
Yes I've taken these from the autodiff class. As I tested the errors calculated by the autodiff class against hand calculations in each of the following tests, I had confidence in these values: Framework/Algorithms/test/PolarizationCorrections/PolarizationCorrectionsHelpersTest.h
Given this, and the fact the signal remains the same as before, do you agree?
There was a problem hiding this comment.
If you ran the numbers with another method and everything agrees, then it is fine for me, perhaps we could mention it in a comment ?
There was a problem hiding this comment.
I haven't run these numbers against another method, though I tested the errors calculated against hand calcs using the algorithm in general against other functions in the unit tests.
github-project-automation
bot
moved this from Review
to In Progress
in ISIS LSS Sprint Planning
Thanks, will have to look in to this tomorrow. I seem to be able to reproduce a different seg fault, it's an access violation in a unexpected location, seemingly during I replicated this using data stored locally. Mantid logs until fault: EDIT: Also |
actually remove unintended change actually actually remove unintended change
MialLewis
force-pushed
the
37768_improve_error_calc_in_PolarizationEfficienciesWildes
branch
from
54379fc to
a62caec
Compare
adriazalvarez
previously approved these changes
May 22, 2025
cailafinn
deleted the
37768_improve_error_calc_in_PolarizationEfficienciesWildes
branch
github-project-automation
bot
moved this from In Review
to Done
in ISIS Polarised Reflectivity epic
github-project-automation
bot
moved this from In Progress
to Done
in ISIS LSS Sprint Planning
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of work
Summary of work
This PR corrects the error propagation for the
PolarizationEfficienciesWildesalgorithm.As part of this work a class
ErrorPropagationhas been added toPolarizationCorrectionsHelpers.h. This generic class utilizes the eigenAutoDiffmodule to automatically calculate the partials for different input variables. If there is wider interest in this class I will move it into thekernelin a separate PR.The
PolarizationEfficienciesWildesalgorithm has been refactored slightly to enable use of theErrorPropagationclass and to remove the use of derived quantities in workspace arithmetic.The theory behind the approach to error propagation can be read: #37768
Fixes #37768
To test:
ErrorPropagationclass inPolarizationCorrectionsHelpersTest. These include simple examplestestErrorPropagation_linear,testErrorPropagation_quadandtestErrorPropagation_mult_varsthat prove the function of the class. You can check these on paper/using wolfram alpha. The latter test I copied from this blog, obtaining the same results: https://michaelspieler.com/post/eigen-autodiff/PolarizationEfficienciesWildeson some real (system test) data:Reviewer
Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.
Code Review
Functional Tests
Does everything look good? Mark the review as Approve. A member of
@mantidproject/gatekeeperswill take care of it.Gatekeeper
If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.