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

Refactor expansion of elements to nuclides from blueprints #320

Open
jakehader opened this issue Jun 23, 2021 · 4 comments
Open

Refactor expansion of elements to nuclides from blueprints #320

jakehader opened this issue Jun 23, 2021 · 4 comments
Labels
architecture Issues related to big picture system architecture bug Something is wrong: Highest Priority

Comments

@jakehader
Copy link
Member

jakehader commented Jun 23, 2021

When loading a reactor there are functions/methods defined in the blueprints that result in elements being expanded into nuclides using the definition of their natural abundances. This process is sensitive to user settings within the nuclideFlags option in the blueprints as well as some user-defined "magic" settings defined in the autoSelectElementsToKeepFromSettings function:

def autoSelectElementsToKeepFromSettings(cs):

Two issues that I am running into with this are:

  1. The code is hidden and it's challenging from a user-perspective to understand why in certain instances elements are being expanded and why in other instances of elements are not (see: _resolveNuclides). Even with lots of digging and sort of knowing what to look for this took me ~10-15 minutes to find and understand.

  2. There is a different behavior with elemental expansion when loading from inputs (blueprints) versus loading from a database. One may argue that it is not the database's responsibility to perform the nuclide expansion and I would agree. Rather than this code being on the database, it should instead be in the client plugin code that is expecting nuclides to be expanded. I think that the same can be said about loading from the inputs as well (unless explicit expandTo options are set in the nuclideFlags section).

To fix this, I suggest the following:

  1. We introduce some form of plugin hook that can define requirements for expanding nuclides based on the uses.
  2. We don't permanently change the state of the reactor, core, etc. when we expand nuclides (except for when the expandTo options are explicitly set).
  3. We refactor the elemental expansion code to a place in the framework where client code can easily access it and make this elemental expansion modifications to any ArmiObject.

I think that this will help make this behavior much less surprising to a user/developer and will also allow us to move away from these legacy "settings-based" changes to the reactor state within the framework. In this way, client code can still make new settings to configure the expansion of elements, but this will be on a case by case basis and for certain defined situations.

As an example, the situation defined above becomes problematic when performing database loads from a case that used ENDF/B-VII.0 data (where carbon, vanadium, etc.) are not expanded and the data library is switched to say ENDF/B-VII.1 or ENDF/B-VIII.0 where the carbon, vanadium, and other nuclides must be expanded. I think that the elemental expansion methods may make sense to be included in the newer nuclides implementation that is being worked on.

What do we think, @youngmit and @ntouran?

@jakehader jakehader added architecture Issues related to big picture system architecture bug Something is wrong: Highest Priority labels Jun 23, 2021
@youngmit
Copy link
Contributor

In general, I think we should be punting the decision to expand elements to isotopics closer to the specific plugins that carry such requirements. However, ARMI should make imposing those mappings really easy for the plugin developer to implement. One approach could be to add an optional filter argument to getNumberDensities() that ARMI can call on the number densities dictionary before returning it to the caller. This could either be an instance of a class that ARMI provides with desired elemental expansions defined, or a super-custom callable that does more fancy mappings. Then the plugin could do something like:

class FancyCodeInputWriter:
    nDensFilter = armi.reactor.ElementExpander(["C", "Be"])
    ...
    def writeInput(self, r):
        ...
        nDens  = block.getNumberDensities(filter=self.nDensFilter)

The upside here is that the plugin code is far less sensitive to the inputs that originally created a reactor object, and removes the need for the user to manually resolve the mutual elemental requirements of the codes that they wish to use. One wrinkle here is how we should handle situations where we wish to update the number densities within a plugin. In this case, the plugin is making a pretty big decision for everyone else about which isotopes/elements to represent in the reactor model.

@ntouran
Copy link
Member

ntouran commented Jun 23, 2021

I like Mitch's suggestion a lot. From my understanding, the plugins that are writing information wouldn't have to choose which nuclides to represent, that'd be universal. They would have to choose how to remap the elements/isotopes combos that they have updated back onto the unchanging set of nuclides/elements.

Big risk here is what would happen if a plugin inconsistently used the filters. That's another thing to keep in mind for functions like:

  • get/setNumberDensities
  • getNuclides
  • getNulide...
  • get/setMass
  • more?

I guess if these all failed if someone tried to get or set one of the ones that only the plugin knows about mapping that'd be fine.

@john-science
Copy link
Member

@jakehader @ntouran Heya! @keckler were just trying to decide what you guys thought of the expandTo nuclide flag. From the outside, separated by time from when this was implemented, it seems to generate some non-physical ideas.

Is it something like "MC2 doesn't have cross sections for this nuclide, so we're going to exclude it up front"? If so, I feel like ARMI shouldn't be doing that excluding, but the downstream... "mc2 ARMI Plugin" should be doing that.

From the discussion here, it really looks like Mitch suggested exactly that.

@jakehader
Copy link
Member Author

Hey @john-science - I think that the global state of nuclides and needing the set of nuclides initialized at the start of reactor data model construction makes it difficult to parse down the expansion of nuclides in a given plugin or area of interest. For example, the nuclides on the reactor data model, if not complete for depletion later in life then results in the parameter definitions expanding. Maybe this isn't an issue if we rethink this, but this could lead to an inconsistency between the input to codes and the reactor data model being a source of truth.

With that said, the nuclides flags and expansions could be handled by plugins or at an application level versus assumed as inputs from the data model and that might make it easier to configure.

The issue in #1817 doesn't seem good though.

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 bug Something is wrong: Highest Priority
Projects
None yet
Development

No branches or pull requests

4 participants