-
Notifications
You must be signed in to change notification settings - Fork 128
Fix bug polaris total scattering normalisation for multi-atom unit cells #37814
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
Fix bug polaris total scattering normalisation for multi-atom unit cells #37814
Conversation
Choice of number density is arbitrary, it is not used to calculate the cross-sections required here.
As the test failures don't seem to be related to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is straight forward, looks good.
However, I am trying to run the script but I think I am getting an unrelated error:
ValueError: You must set multiple_scattering=True and do_absorb_corrections=True when creating the vanadium run.
So I tried running it with both of the arguments set to True but I get another error later down the line:
NotImplementedError: Multiple scattering absorption corrections are not yet implemented for anisotropic samples
So I am not sure how I can bypass this?
Sorry @GuiMacielPereira - I copied the wrong testing instructions! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good and the script now runs without problems 👍
Looks like there are some failing tests, so I will unassign myself for now |
4085f09
to
023b9b3
Compare
023b9b3
to
b55e7ea
Compare
def validate(self): | ||
self.tolerance_is_rel_err = True | ||
self.tolerance = 1e-6 | ||
self.tolerance = 1e-5 # same tolused in FocusTestAbsorptionMayers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.tolerance = 1e-5 # same tolused in FocusTestAbsorptionMayers | |
self.tolerance = 1e-5 # same tol used in FocusTestAbsorptionMayers |
# define multi-atom cell to stop regression of bug calling MaterialBuilder.build() without setting number density | ||
# which works for one atom cells as number density is automatically calculated | ||
sample_details.set_material(chemical_formula="Si Si", number_density=0.04996) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit confused, does this mean that now users have to change how they set the chemical_formula
argument to get the same behaviour as before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, users will set the material as/how they always have. Here I specify the chemical formula as "Si Si"
because mantid has a lookup somewhere in MaterialBuilder
to find the number density if you specify a single atom formula (e.g "Si"
) which meant the tests weren't catching the bug found by POLARIS scientists (with real samples containing more atom species). The two formula "Si Si"
and "Si"
should be equivalent as the default unit for number density in mantid is (for better or worse) atoms/Ang^3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, thanks for clarifying!
Description of work
There was a bug in the call to MaterialBuilder here
mantid/scripts/Diffraction/isis_powder/polaris_routines/polaris_algs.py
Line 107 in 2ae50d1
The
build
method requires unit cells with multiple atoms to have a number density set.The number density is not actually used to calculate the cross-sections required for the PDF normalisation - so here I just set it to an arbitrary number (1).
There is no associated issue.
Report to: Helen Playford
To test:
(1) Unzip these files somewhere and update the script below with the path. Also update the paths in the file polaris_config_example.yaml PR34144Files.zip
(2) Run this script without error
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.