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

Move a/b.p.detailedNDens to internal plugin #1702

Open
keckler opened this issue May 16, 2024 · 8 comments
Open

Move a/b.p.detailedNDens to internal plugin #1702

keckler opened this issue May 16, 2024 · 8 comments
Labels
cleanup Code/comment cleanup: Low Priority

Comments

@keckler
Copy link
Member

keckler commented May 16, 2024

The parameter detailedNDens is defined for both blocks and assemblies in the framework. Here are both of those definitions:

pb.defParam(
"detailedNDens",
setter=isNumpyArray("detailedNDens"),
units=f"atoms/(bn*{units.CM})",
description=(
"High-fidelity number density vector with up to thousands of nuclides. "
"Used in high-fi depletion runs where low-fi depletion may also be occurring. "
"This param keeps the hi-fi and low-fi depletion values from interfering. "
"See core.p.detailedNucKeys for keys. "
# Could be moved to external physics plugin
),
saveToDB=True,
default=None,
)

pb.defParam(
"detailedNDens",
setter=isNumpyArray("detailedNDens"),
units=f"atoms/(bn*{units.CM})",
description=(
"High-fidelity number density vector with up to thousands of nuclides. "
"Used in high-fi depletion runs where low-fi depletion may also be occurring. "
"This param keeps the hi-fi and low-fi depletion values from interfering. "
"See core.p.detailedNucKeys for keys. "
# could move to external physic plugin
),
location=ParamLocation.AVERAGE,
saveToDB=False,
default=None,
)

There are a couple minor issues with these parameters being defined here:

  1. They only make sense within the context of the internal TP depletion solver, and are thus confusing to anyone that doesn't have access to that internal solver
  2. These parameters are only set if a specific setting is turned on (burnxDetailedDepletion), and that setting is internal to TP's ARMI app
  3. This parameter is seemingly duplicate with the other number density block parameters (nFe56, nU238, etc.) and is thus confusing

In order to disambiguate the situation, it would be great if we could add context to the parameter's description that, for instance, it is only set when burnxDetailedDepletion is turned on. In order to do that, the setting would have to be made internal.

I think there would be value in bringing this hyper-specific setting internal. The setting definition says as much in a comment, so I don't anticipate this would break anything.

@john-science john-science added the cleanup Code/comment cleanup: Low Priority label May 16, 2024
@john-science
Copy link
Member

The question in ARMI is always "Is this idea generally useful to reactor modeling". Now, maybe some ideas are specific to LWRs or SFRs or hex-assemblies, but those are still worth including in ARMI for a good chunk of the target audience. In that light, this Parameter feels pretty borderline to me; maybe someone else would want this Parameter. But, this was clearly designed with a very specific implementation of our internal code in mind.

I agree with the proposal; it feels cleaner to put this Parameter with the depletion code that it was designed for.

@albeanth
Copy link
Member

In that light, this Parameter feels pretty borderline to me; maybe someone else would want this Parameter.

I've always thought that ARMI had too many parameters that aren't actually tied to anything in ARMI. We just assume that people will want to use them (which I think is a questionable assumption, personally). I imagine that most people, if they're taking the time and effort to write their own ARMI Application, will want to write their own parameters and not rely on the defaults in ARMI.

I think it would be cleaner (and easier to maintain by not having to ask permission or surprisingly wrecking someone's App) if we only left the most obvious ones in ARMI (e.g., flux, keff, pitch, other geometric info, etc) or ones that are actually used in ARMI. These could serve as examples for others to develop their own.

Thoughts?

@keckler
Copy link
Member Author

keckler commented May 20, 2024

I've always thought that ARMI had too many parameters that aren't actually tied to anything in ARMI. We just assume that people will want to use them (which I think is a questionable assumption, personally). I imagine that most people, if they're taking the time and effort to write their own ARMI Application, will want to write their own parameters and not rely on the defaults in ARMI.

I very much agree with this sentiment. It is not difficult for the creator of an App to define their own parameters. I think that stuffing the open-source ARMI with maybe-useful parameters is only to our detriment.

@john-science
Copy link
Member

I think it would be cleaner iif we only left the most obvious ones in ARMI

Agreed. So far, we have removed ~150 Parameters from ARMI. But these are breaking changes, and some of those parameters are used downstream, so they have to be moved carefully rather than just deleted. I was able to just straight delete 100-120 parameters, but that did take weeks of discussion to do.

If there is support, I would be happy to do more. Perhaps there are bulk groupings of parameters we could consider all at one time. Say, if there was a particular TH or neutronics application that wanted to take responsibility for a bunch of ARMI parameters and move them out of ARMI... that could be really helpful. :)

@albeanth
Copy link
Member

Say, if there was a particular TH or neutronics application that wanted to take responsibility for a bunch of ARMI parameters and move them out of ARMI... that could be really helpful. :)

I'll start an internal discussion!

@john-science
Copy link
Member

Okay, after some discussion, we've decided to just move this parameter out of ARMI.

@john-science
Copy link
Member

Also, the detailedNDens param and the detailedNucKeys parameter are completely tied together, and can not be separated.

@john-science
Copy link
Member

Drat.

This parameter is actually used directly in ARMI:

armi/armi/reactor/blocks.py

Lines 727 to 745 in 9ea428b

def _updateDetailedNdens(self, frac, adjustList):
"""
Update detailed number density which is used by hi-fi depleters such as ORIGEN.
Notes
-----
This will perturb all number densities so it is assumed that if one of the active densities
is perturbed, all of htem are perturbed.
"""
if self.p.detailedNDens is None:
# BOL assems get expanded to a reference so the first check is needed so it
# won't call .blueprints on None since BOL assems don't have a core/r
return
if any(nuc in self.core.r.blueprints.activeNuclides for nuc in adjustList):
self.p.detailedNDens *= frac
# Other power densities do not need to be updated as they are calculated in
# the global flux interface, which occurs after axial expansion from crucible
# on the interface stack.
self.p.pdensDecay *= frac

It is called in one place:

armi/armi/reactor/blocks.py

Lines 685 to 706 in 9ea428b

def adjustDensity(self, frac, adjustList, returnMass=False):
"""
adjusts the total density of each nuclide in adjustList by frac.
Parameters
----------
frac : float
The fraction of the current density that will remain after this operation
adjustList : list
List of nuclide names that will be adjusted.
returnMass : bool
If true, will return mass difference.
Returns
-------
mass : float
Mass difference in grams. If you subtract mass, mass will be negative.
If returnMass is False (default), this will always be zero.
"""
self._updateDetailedNdens(frac, adjustList)

So, this probably isn't a simple copy/paste any more.

Let me look at this.

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

No branches or pull requests

3 participants