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

Abins: improve handling of isotopic cross-sections #38448

Merged
merged 17 commits into from
Jan 7, 2025

Conversation

ajjackson
Copy link
Contributor

@ajjackson ajjackson commented Nov 26, 2024

Description of work

Summary of work

  • introduce unit test that error should be raised for Zn65
  • modify code to raise ValueError in such cases
  • introduce unit test that Zn_30_65.39 should be identified as isotopic mixture
  • modify code to detect when standard mass is used

Purpose of work

Primary issue:
Unstable isotopes such as Zn65 currently give a NaN cross-section. We should ensure that unsupported isotopes give a noisy error instead.

Secondary issue:
We are seeing Zn65 because data with mass 65.39 is being inappropriately rounded to this exotic isotope, rather than identified as being the standard isotopic mixture. Such cases should be detected more robustly.

Fixes #37735

Report to: Jeff Armstrong

Further detail of work

To test:

The changes are covered by unit tests and system tests


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

  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards?
  • Are the unit tests small and test the class in isolation?
  • If there is GUI work does it follow the GUI standards?
  • If there are changes in the release notes then do they describe the changes appropriately?
  • Do the release notes conform to the release notes guide?

Functional Tests

  • Do changes function as described? Add comments below that describe the tests performed?
  • Do the changes handle unexpected situations, e.g. bad input?
  • Has the relevant (user and developer) documentation been added/updated?

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.

This isotope currently gives a NaN cross-section. We should ensure
that unsupported isotopes give a noisy error instead.
@ajjackson ajjackson added the ISIS Team: Spectroscopy Issue and pull requests managed by the Spectroscopy subteam at ISIS label Nov 26, 2024
@ajjackson
Copy link
Contributor Author

ajjackson commented Nov 26, 2024

Currently Abins identifies if the mass is a standard isotopic mixture by comparing against Atom(symbol=symbol).mass with a threshold of 0.01.

In the case of zinc, the standard mass has been revised by a difference of more than 0.01, so CASTEP writes mass values that are not detected by Abins as being the standard.

A more robust workflow could look at which is nearest between the standard mass and another known isotope.

I have found a quirk in the Atom API though:

In [44]: Atom(symbol="Zn").mass
Out[44]: 65.409

In [45]: Atom(symbol="Zn", a_number=82).mass
Out[45]: 81.95484

In [46]: Atom(symbol="Zn", a_number=83).mass
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
Cell In[46], line 1
----> 1 Atom(symbol="Zn", a_number=83).mass

RuntimeError: Failed to find an atom with symbol=Zn and a=83

In [47]: Atom(symbol="Zn", a_number=20).mass
Out[47]: 53.99295

Mass values that are too high raise an error, but masses that are too low do not!

- remove "substitution" logic branch
- instead delegate isotope detection and workspace naming to a new class
- in this commit a new Atom is still (unnecessarily) spawned to get cross-section
We can now pass on the AtomInfo object rather than its consitituent data.
In cases such as F, one isotope is so dominant that essentially we
would always use the properties of the standard mixture. Oddly, in
this case Mantid will not find neutron data if the atomic mass was
provided.

i.e. Atom('F').neutron() works, but Atom('F', 19).neutron() returns a
dict full of NaN, despite them having exactly the same mass.
Abins input files usually give the element symbol and mass. These
masses often do not match exactly to those in Mantid's data.

- In the case that the nearest isotope has no data, but the "mixture"
  is _quite_ near the target mass (0.01 amu, for now), log a warning and
  use the mixture.

  This helps handle nasty cases like 127I where Mantid has _slightly_
  different masses for the pure isotope and mixture, but essentially
  it is a 100% mixture and there is no neutron-scattering data for the
  isolated isotope.

- If the nearest isotope has no data and the standard mixture is too
  far away, raise an error.
@ajjackson
Copy link
Contributor Author

ajjackson commented Nov 28, 2024

For an example of a really nasty case that requires some logic to get through:

  • Mantid's Atom.cpp defines a standard I53 with mass 126.904470 and, among many others, an isotope 127I53 with abundance 100% and mass 126.904468
  • The mass assigned to "standard" iodine by CASTEP is 126.903999; presumably these codebases are using reference data from different years.
  • So: In a real-world case, Abins needs to look for a scattering cross section given user input of "Zn" and 126.903999.

To resolve this, we

  • check if the requested mass is closer to the Mantid reference mass of 127I53 or I53;
  • in this case the 127I53 mass is actually closer so we assume this could be what they wanted.
  • Then we check if data is available; the coherent scattering length is NaN so it is not.
  • Because we are within 0.01 of the standard mass, we assume this was the original intent, raise a warning, and use the I53 data.

The error is now raised by the data class when we try to access a
property that uses Atom.

As such it is not really a get_cross_section test any more and can be
moved out.
@ajjackson ajjackson marked this pull request as ready for review November 28, 2024 16:43
@ajjackson
Copy link
Contributor Author

The docs failure might be connected to

17:04:07 release/v6.12.0/indirect_geometry:3: WARNING: Bullet list ends without a blank line; unexpected unindent. [docutils]

but I'm not sure what to do about that. The pre-commit hooks will not let me append another newline to the release note!

@ajjackson
Copy link
Contributor Author

Thanks @robertapplin for figuring out the docs formatting 🎉

@sf1919 sf1919 added this to the Release 6.12 milestone Jan 3, 2025
Copy link
Contributor

@adriazalvarez adriazalvarez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes are sensible and adding the atominfo dataclass handles better edge cases and also simplifies the code in some parts. Additionally, test cases are added.
I have very small comments but otherwise looks fine to me.

scripts/abins/abinsalgorithm.py Outdated Show resolved Hide resolved
scripts/abins/abinsalgorithm.py Outdated Show resolved Hide resolved
scripts/abins/abinsalgorithm.py Outdated Show resolved Hide resolved
ajjackson and others added 3 commits January 6, 2025 10:09
This gets rid of a hard-coded epsilon. Strictly these two comparisons
are not quite doing the same thing, but this precision should be good
for both.
Copy link
Contributor

@adriazalvarez adriazalvarez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing my comments.

@SilkeSchomann SilkeSchomann self-assigned this Jan 7, 2025
@SilkeSchomann SilkeSchomann merged commit 89cf16e into mantidproject:main Jan 7, 2025
10 checks passed
peterfpeterson pushed a commit to peterfpeterson/mantid that referenced this pull request Jan 17, 2025
This is a squashed version of mantidproject#38448

Create failing test: Abins get_cross_section for Zn65

This isotope currently gives a NaN cross-section. We should ensure
that unsupported isotopes give a noisy error instead.

Implement ValueError for NaN cross-section data

Fix missing return value, test good cases

Begin refactoring Abins isotope handling

- remove "substitution" logic branch
- instead delegate isotope detection and workspace naming to a new class
- in this commit a new Atom is still (unnecessarily) spawned to get cross-section

Continue refactor: streamline _create_workspace

We can now pass on the AtomInfo object rather than its consitituent data.

Continue refactor: move AtomInfo into _fill_s_workspace methods

Simplify get_cross_section from an established AtomInfo

Handle neutron data for atoms where one isotope = mixture

In cases such as F, one isotope is so dominant that essentially we
would always use the properties of the standard mixture. Oddly, in
this case Mantid will not find neutron data if the atomic mass was
provided.

i.e. Atom('F').neutron() works, but Atom('F', 19).neutron() returns a
dict full of NaN, despite them having exactly the same mass.

Update Abins2D to use AtomInfo class

Handle some more Abins isotope challenging cases

Abins input files usually give the element symbol and mass. These
masses often do not match exactly to those in Mantid's data.

- In the case that the nearest isotope has no data, but the "mixture"
  is _quite_ near the target mass (0.01 amu, for now), log a warning and
  use the mixture.

  This helps handle nasty cases like 127I where Mantid has _slightly_
  different masses for the pure isotope and mixture, but essentially
  it is a 100% mixture and there is no neutron-scattering data for the
  isolated isotope.

- If the nearest isotope has no data and the standard mixture is too
  far away, raise an error.

Direct unit test for AtomInfo

Test refactor: move error check closer to source

The error is now raised by the data class when we try to access a
property that uses Atom.

As such it is not really a get_cross_section test any more and can be
moved out.

Add release note

Tweak release note format

Update scripts/abins/abinsalgorithm.py

Co-authored-by: Adri Diaz <[email protected]>

Move AtomInfo to separate file

Tighten epsilon for "same" mass values, re-use in atominfo

This gets rid of a hard-coded epsilon. Strictly these two comparisons
are not quite doing the same thing, but this precision should be good
for both.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ISIS Team: Spectroscopy Issue and pull requests managed by the Spectroscopy subteam at ISIS
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

Abins: NaNs quietly propagate through results if isotope data unavailable
4 participants