-
Notifications
You must be signed in to change notification settings - Fork 126
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
Update pychop definitions for SNS instruments #38591
base: main
Are you sure you want to change the base?
Conversation
@mducle Please add a release note in \docs\source\release\v6.12.0\Direct_Geometry\General\Bugfixes |
I notice that in the .yaml for SEQUOIA, the comment on moderator parameters has not changed and indicates that B will be used directly (rather than as reciprocal). I compared these comments a bit more closely with the code, and neither of them seems to be an accurate description of the Lines 116 to 129 in 6ee0b15
Specifically:
refs: |
I'll see if I can dig out where the approximation comes from... it's probably more fully explained in the original Fortran code, which I have... somewhere... Also the unit test is broken because it compares numerical values of the resolution and flux which is obviously now different (but correct!). I'll fix that and add release notes. |
@ajjackson The 81.81 is the conversion from energy in meV to wavelength in Angstroms. import scipy.constants as sc
lam2meV = sc.Planck**2/(2*sc.e*sc.m_n*1e-23) |
@ajjackson The original Fortran code is in a gist here. Not sure it's super useful. I think that the value returned by the function, |
@AndreiSavici I've changed the For ARCS I've included a scale factor of 600 for the flux and for SEQUOIA I've changed the look up table to include the detector efficiency and absorption corrections which the website code does. So now Mantid-PyChop will give the same flux (in n/cm^2/s/MW) as the webpage. @SilkeSchomann I've also changed the documentation to explicitly say that the flux values given for ISIS and SNS instruments are in different units. (Although in principle if both facilities are running at full power the flux should be in the same units of n/cm^2/s). I haven't changed the GUI - it still says n/cm^2/s - do you think that's needed for this? |
@mducle I would change the GUI as well. A mismatch between GUI and documentation will probably confuse the user. |
Description of work
When SNS instruments were added to PyChop around 2020 I used the original
.yaml
files rather than fine-tuned files which Jiao Lin had created during the work for this publication. So, the calculations in Mantid has actually been incorrect for 4 years!During work for the resconvlib project @RastislavTuranyi found that Mantid calculations disagreed with the online calculator provided by SNS which is also meant to be using PyChop. Furthermore the Mantid calculations disagreed drastically with the measured data in the ARCS resolution paper.
The discrepancy is because the moderator parameters (amongst other things) were incorrect in the initial version added to Mantid.
Summary of work
This PR is to update these
.yaml
files to the optimised files in Jiao Lin's original repository, namely the files:There is no associated issue.
Further detail of work
To test:
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/gatekeepers
will 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.