Following the addition of McStas examples and user guide notebooks to FREIA there were some comments to the PR that were agreed should be addressed in a followup:
Gravity correction reuse
ScippNeutron's gravity correction.
Is this something that is already implemented in scippneutron or is it just an example copied from the docs?
If the former, can we import from scippneutron and use here instead of re-implementing?
There is a similar implementation of this in Amor (I think?). Can we change them to a common function?
Add inspector plot
Is it worth adding at the end of the notebook an inspector plot, where we histogram in longitude, height and wavelength? And then draw some rectangles around some regions?
Use CorrectForGravity sciline parameter
In esssans, we have bool parameter which is called CorrectForGravity (see here).
Can we do the same here?
Suggestion to rename prepare_sample function
'prepare' is a little vague. Can we use a name which is more descriptive?
Document procedure for processing direct beam
(About explanation in normalization by direct beam)
The explanations should go close to the code (either comment or docstring)
add_masks fix interface
The function currently prefers masking event coords, if the user intended to mask a coord on the binning with the same name that might result in an error.
As it is currently used in a workflow that is not a problem, because the data it is applied to has no coords that live on both levels.
But if used elsewhere it might lead to a bug in that context.
Either mark it as private or change the interface to raise an error when the coord is ambiguous.
It's also strange that it takes the WavelengthBins domain type, that domain type should probably be removed from the worflow for now.
Following the addition of McStas examples and user guide notebooks to FREIA there were some comments to the PR that were agreed should be addressed in a followup:
Gravity correction reuse
Is this something that is already implemented in scippneutron or is it just an example copied from the docs?
If the former, can we import from scippneutron and use here instead of re-implementing?
There is a similar implementation of this in Amor (I think?). Can we change them to a common function?
Add inspector plot
Is it worth adding at the end of the notebook an inspector plot, where we histogram in longitude, height and wavelength? And then draw some rectangles around some regions?
Use
CorrectForGravitysciline parameterIn esssans, we have bool parameter which is called CorrectForGravity (see here).
Can we do the same here?
Suggestion to rename
prepare_samplefunction'prepare' is a little vague. Can we use a name which is more descriptive?
Document procedure for processing direct beam
(About explanation in normalization by direct beam)
The explanations should go close to the code (either comment or docstring)
add_masksfix interfaceThe function currently prefers masking event coords, if the user intended to mask a coord on the binning with the same name that might result in an error.
As it is currently used in a workflow that is not a problem, because the data it is applied to has no coords that live on both levels.
But if used elsewhere it might lead to a bug in that context.
Either mark it as private or change the interface to raise an error when the coord is ambiguous.
It's also strange that it takes the
WavelengthBinsdomain type, that domain type should probably be removed from the worflow for now.