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

Make NuclideBases not global #473

Open
youngmit opened this issue Nov 12, 2021 · 8 comments
Open

Make NuclideBases not global #473

youngmit opened this issue Nov 12, 2021 · 8 comments
Assignees
Labels
architecture Issues related to big picture system architecture cleanup Code/comment cleanup: Low Priority

Comments

@youngmit
Copy link
Contributor

The directory in nuclideBases is global, which leads to considerable pain:

  • Import order is very sensitive, and lots of things have to happen at import time, and cant happen again or else 💥.
  • The actual contents change when a burn chain is imposed, which is really case dependent and can only happen once, or else 💥
  • Some code changes the LFP properties, and these changes probably don't survive round-tripping through the database. Not exactly related to global state, but part of the reason we haven't been thinking of it. See collapseFissionProducts and updateYieldVector methods within the fissionProductModel.

So, we should be thinking of another place for this to live, and spin up an object containing the valid nuclides for that scope. Reactor? Case? Somewhere else? Even a global database that is actually an object instead of a magical singleton-esque horror would be better than it is now. Also, if it is mutable beyond the initial setup with a burn chain, then we need to think about how to recover it's state from the database.

@youngmit youngmit added architecture Issues related to big picture system architecture cleanup Code/comment cleanup: Low Priority labels Nov 12, 2021
@jakehader
Copy link
Member

We made a step in a direction where global nuclide bases are less bad than they used to be by adding more nuclides in #998. Unfortunately, the nuclide data is still global and the burn chain modifications still may occur when it's being imposed. I specifically added logging statements if a user overwrites any default parameter from the burn-chain.yaml or custom file, but the transmutation and decay data that is a subset of the nuclides still gets modified based on the user-defined burn chain data.

We can probably take this further still.

@john-science john-science self-assigned this Apr 4, 2024
@john-science
Copy link
Member

john-science commented Apr 4, 2024

@opotowsky

This has reared it's head again, and will have to be put on the calendar for ARMI 0.4.0. It turns out ARMI enforces everyone to use RIPL nuclear data, but we have some users that use ENDF in other parts of their modeling chain. So I would LOVE to give people a setting to choose which nuclear data they want. But we can't do that (easily) if the nuclear data lives in the global scope at import time.

So, finally, I think we have enough interest to make this ticket happen.

@john-science
Copy link
Member

john-science commented Oct 23, 2024

I notice that it is not just nuclideBases that is global, but also elements and thermalScattering. We even have an unrelated ticket open to rip thermalScattering out of the global scope.

So, let's try to itemize the nuclide information that is currently in the global scope, and where (outside the nucDirectory area) it is used:

nuclideBases

Name Usage
byName * very popular in downstream projects
* various materials
* NuclideFlag
* component
* composite
* densityTools
byDBName * one downstream plotting function
byLabel * two downstream projects
* Uranium.setDefaultMassFracs
* xsNuclides.updateBaseNuclides
byMcc2Id * unused
byMcc3Id * downstream dif3d and depletion codes
* dlayxs
* isotopicOptions.getAllNuclideBasesByLibrary
byMcc3IdEndfbVII0 * only used in this file
byMcc3IdEndfbVII1 * only used in this file
byMcnpId * downstream burnx and mcnp code
byAAAZZZSId * single isotxsSample file in nucleardata-uq

elements

Name Usage
byZ * blocks::getPuMoles
* isotopicOptions::autoUpdateNuclideFlags()
* Composite.constituentReport()
byName * isotopicOptions.eleExpandInfoBasedOnCodeENDF(cs)
bySymbol * many places downstream
* isotopicOptions::ALLOWED_KEYS and .expandMassFracs
* Composite::getNuclidesFromSpecifier and .constituentReport()
* Water.setDefaultMassFracs

thermalScattering

Name Usage
byNbAndCompound * materials like BE9 and Graphite
* OpenMC plugin

@john-science
Copy link
Member

john-science commented Oct 23, 2024

Materials

I am a touch worried about the current state of ARMI materials because of this "global nuclides" situation. Currently, if you update the burn chain, all the global nuclides can be updated. Which means that the mass fractions of materials can be changed in the middle during a run?

That is a dangerous idea in parallel unit tests.

@john-science
Copy link
Member

john-science commented Nov 22, 2024

@aaronjamesreynolds @opotowsky And anyone else who is interested... I have a branch out now to start this this ticket: obliterate_global_nuclides.

(To see the current state of the branch go here.)

All these global will be moved into this new class:

class NuclideBases:

Note the big table above. This is only the start. But merge freeze or not, I can start thinking about all this.

@jakehader
Copy link
Member

This looks good @john-science. Are you thinking that this class would be instantiated on an application basis and then once the application is configured we can call through app.nuclides or did you have something else in mind for how it should be connected and configurable?

@john-science
Copy link
Member

john-science commented Nov 22, 2024

Are you thinking that this class would be instantiated on an application basis ...

Well, at the moment, I am guessing the nuclides bases have to live on the Reactor, so r.nuclideBases. But that's just a guess. The truth is the solution will have to derive from the table above.

That table shows where the nuclide bases are used. So, the usage of x.nuclideBases will determine where they have to live. So, here can we put this new object that causes the fewest changes in ARMI and downstream?

It is probable that no matter where I put the new object, there will have to be a some breaking changes (probably in the materials). BUT choosing a good place to put this new object will cut the work needed for this change down by 2x or even 10x.

@drewj-tp
Copy link
Contributor

Two cents: R.nuclides over App.nuclides

Rationale: interfaces always have access to the reactor, not necessarily the application. And the app is already a global thing, so putting the nuclides there isn't really getting rid of global nuclides. It's just moving them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture Issues related to big picture system architecture cleanup Code/comment cleanup: Low Priority
Projects
None yet
Development

No branches or pull requests

4 participants