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

Use ENDF/B-VII.1 MC2-3 nuclide IDs #1982

Merged
merged 31 commits into from
Nov 8, 2024

Conversation

aaronjamesreynolds
Copy link
Contributor

@aaronjamesreynolds aaronjamesreynolds commented Oct 28, 2024

What is the change?

Currently, ARMI returns MC2-3 nuclide IDs consistent with the ENDF/B-VII.0 library. This PR makes it so that:

  • ENDF/B-VII.1 nuclides and MC2-3 IDs are added to mcc-nuclides.yaml,
  • the default MC2-3 ID returned (e.g., when using getMcc3Id() or indexing the byMcc3Id dictionary) is consistent with ENDF/B-VII.1, and
  • the VII.0 IDs are still accessible by means of getMcc3IdEndfVII0() or indexing the byMcc3IdEndfVII0 dictionary.

This paper provides a good overview of the differences between VII.0 and VII.1.

Tests have been updated or created to verify that getMcc3Id(), getMcc3IdEndfVII0(), and getMcc3IdEndfVII1() return the expected IDs. Additionally, two tests (test_compareDatabaseSim() and setUp() [for the cross section manager]) needed to be updated since vanadium is elemental in VII.0 and isotopic in VII.1.

Why is the change being made?

ENDF/B-VII.1 contains updated evaluations and entirely new evaluations compared to ENDF/B-VII.0. ANL has recently released VII.1-based MC2-3 libraries, so the most recent available data is the sensible default.


Checklist

  • The release notes have been updated if necessary.
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in pyproject.toml.

Aaron Reynolds added 13 commits October 10, 2024 23:10
Changes relative to VII.0 are:
* V is no longer elemental (add V-50/51)
* Zn is no longer elemental (add Zn-54/65/66/67/68/70)
* Add Tm-168/169/170
* Add Ta-180
* Add W-180
* Add Tl-203/205
* Add Th-231
* Add Pa-229/230
* Add U-230/231
* Add Np-234
* Add Am-240
* Add Cm-240
* Add Bk-245/246/247/248
* Add Cf-246/248
* Add Es-251/252/254m

So 32 new isotope evaluations and the elemental evaluation of V and Zn
are gone in VII.1.
To support the VII.1 MC2-3 library, ARMI needs a lookup dictionary by
library ID. Previously, there was just a single MC2-3 dictionary
to do this, but now we need two - one for the VII.0 library and one
for the VII.1 library. The preexisting `byMCC3Id` dictionary is still
availabe and now returns VII.1 data.

Also:
* adds getter functions for the VII.0 and VII.1 IDs on a couple classes
* modifies existing versionless getMcc3ID getters to wrap VII.1 getters
* modifies readMCCNuclideData to parse the VII.1 nuclide labels
Previously, python could find the byMcc3Id in the global scope without
needing to declare it in the readMccNuclideData function. After
introducing the new byMcc3IdEndfvii.* dictionaries, it can't anymore.
Declaring all the byMcc3Id* dictionaries as global to be safe.
These new tests are patterned off the old test for loading the MC2-3
data, they just check the dedicated VII.0 and VII.1 dictionaries and
class functions, now.
This hopefully leads to fewer broken integration tests in Nala. If we
point to ENDF/B-VII.1 isotopes, tests break since V is isotopic, rather
than elemental.
This reverts commit 67be3f2. The
sensible default for byMcc3Id is ENDF/B-VII.1 since that's what we're
adopting for the project
This is a wrapper around the function that return the VII.1 MC2-3 ID
Previously ARMI assumed elemental data consistent to VII.0. Since we are
transitioning to using VII.1 data by default, this logic needs to be
updated to act on the VII.1 elemental list.
The diff vector was originally expected to be 477. With the change to
ENDF/B-VII.1, elemental vanadium is gone, but two isotopes of vanadium
are added. Each element/isotope has 3 pieces of data, so the new size of
the vector is 477 - 3 * [2 - 1] = 480.
@CLAassistant
Copy link

CLAassistant commented Oct 28, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 3 committers have signed the CLA.

✅ john-science
❌ Aaron Reynolds
❌ aaronjamesreynolds


Aaron Reynolds seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@john-science john-science added the feature request Smaller user request label Oct 28, 2024
@john-science john-science self-requested a review October 28, 2024 20:18
doc/release/0.4.rst Outdated Show resolved Hide resolved
doc/release/0.4.rst Outdated Show resolved Hide resolved
@john-science john-science marked this pull request as ready for review October 28, 2024 20:50
Copy link
Member

@john-science john-science left a comment

Choose a reason for hiding this comment

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

Okay, @aaronjamesreynolds , I have determined that I like your PR. I just hate global nuclides.

I'm sorry it took me so long to get there.

Thanks for contributing!

@@ -553,7 +553,7 @@ def eleExpandInfoBasedOnCodeENDF(cs):
)

elif cs[CONF_XS_KERNEL] in ["", "SERPENT", "MC2v3", "MC2v3-PARTISN"]:
elementalsToKeep.update(endf70Elementals)
elementalsToKeep.update(endf71Elementals)
Copy link
Member

Choose a reason for hiding this comment

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

I feel like SERPENT can be removed from this list since its not tested here. Additionally, doesn't this remove support of ENDF/B-VII.0 explicitly? It feels like there should be a setting for which library is being selected OR we should ensure that that changelog notes that this removes the backwards capability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@john-science, any concerns with removing SERPENT from this list? We could also just make another conditional that has SERPENT default to the endf70Elementals .

Having a setting for the library was my original plan, but global nuclides complicates this. John and I have are hoping to implement such a setting once global nuclides are addressed (#473).

Copy link
Member

@jakehader jakehader Nov 1, 2024

Choose a reason for hiding this comment

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

I understand the complications of the mixed library options based on that ticket and I agree that fixing the global nuclides issue would be a reasonable resolution.

@drewejohnson originally made this setting, so I think it should be appropriate to remove at this time since there is no use for it. I am OK with keeping it as an option though and adding endf70Elementals on that branch or removing SERPENT and updating the changelog.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright Jake, I added a conditional so that VII.0 elementals are used with Serpent.

@opotowsky opotowsky merged commit 864106e into terrapower:main Nov 8, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Smaller user request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants