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

Aguide convention fix #253

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open

Aguide convention fix #253

wants to merge 30 commits into from

Conversation

bmaranville
Copy link
Member

Fix two nearly-self-cancelling errors:

  1. The cross-sections in the magnetic reflecitivity kernel were misidentified as being in the order $(r^{--}, r^{-+}, r^{+-}, r^{++})$ (derivations and original docstrings indicate that they are in the opposite order)
  2. The angle Aguide being sent to the calculation kernel was defined in the opposite sense to EPS in the derivations

Now, we interpret the outputs of the reflectivity kernel as being in the order $(r^{++}, r^{+-}, r^{-+}, r^{--})$, but then reverse them when returning from refl1d.sample.reflectivity.magnetic_amplitude in order to maintain backward compatibility with the order used in PolarizedNeutronProbe objects' cross-sections, and we explicitly pass EPS = -Aguide to the calculation kernel (again, for backward compatibility)

The end result is that 99% of users' models should continue to work without modification (as long as they do not have significant Zeeman-effect-induced splitting in the spin-flip scattering), and models that do show significant Zeeman effects should now work correctly.

The changes are documented in the errata section of the generated documentation, and examples of spin-flip scattering in high fields are provided in the tutorials/validation section of the docs (and validated against the gepore code developed by C. F. Majkrzak et al.)

@bmaranville bmaranville force-pushed the aguide-convention-fix branch from b17560d to 9e6df0d Compare January 29, 2025 15:21
@pkienzle
Copy link
Member

Having an inconsistent order of cross sections within our code base is likely to cause us confusion in the future. Could we instead treat this as a data reader problem, and reverse the order the cross sections are handled in the PNR probe?

Do we need the validation code in the released package? It seems to me that this is code we need to run once to generate the figures for the manual. Is there any reason for a user to run it? Do we need to run it during CI, or could we check against captured output from a single call to validation?

It we expect that a user will want to run gepore fortran code then we should document the process for installing gfortran on their machine.

@bmaranville
Copy link
Member Author

bmaranville commented Jan 31, 2025

Having an inconsistent order of cross sections within our code base is likely to cause us confusion in the future. Could we instead treat this as a data reader problem, and reverse the order the cross sections are handled in the PNR probe?

I think a lot of user scripts interact with the xs by index - I am worried about breaking those, in a way where people wouldn't even know they were broken. How do we mitigate that?

All parts of our code (after the reflectivity is returned from magnetic_amplitude) treat the cross-sections as being in the same order as the existing probe xs.

@bmaranville
Copy link
Member Author

Do we need the validation code in the released package? It seems to me that this is code we need to run once to generate the figures for the manual. Is there any reason for a user to run it? Do we need to run it during CI, or could we check against captured output from a single call to validation?

It we expect that a user will want to run gepore fortran code then we should document the process for installing gfortran on their machine.

I converted the pylit documentation files for the gepore validation to Jupyter notebooks, and committed the (already-run) notebooks, so that you don't need to install/run gepore to generate the docs.

@bmaranville bmaranville force-pushed the aguide-convention-fix branch from 879ce9c to e13c974 Compare February 4, 2025 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants