Improving efficiency of reaction rate calcs

[mgjarrett]

What is the change?

Improve the efficiency of reaction rate calculations.

Why is the change being made?

A common post-processing step for a GlobalFluxInterface is to compute reaction rates from the flux solution and the microscopic cross sections that exist on the reactor model. The implementation of this calculation is done in a straightforward way, but it is naive with-respect-to the typical data structure in an ARMI simulation. The efficiency for typical problems can be greatly improved by refactoring the loops so that the outermost loop is over cross section type.

Often, there are thousands of blocks in a reactor model but only a handful of unique microscopic cross section types; each set of microscopic cross sections might be shared across several hundred blocks. An outer iteration over cross section types with an inner iteration over blocks can significantly reduce the number of times a microscopic cross section set needs to be retrieved.

---

Checklist

• This PR has only one purpose or idea.
• Tests have been added/updated to verify any new/changed code.

• The code style follows good practices.
• The commit message(s) follow good practices.

• 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.

[keckler]

I think the PR probably does what you want it to do, but just added some suggestions to maybe make things more clear.

[keckler]

Should we allow for this function to silently continue if an object without flux is passed in? The explicit purpose of this function is to calculate reaction rates, so passing an object without flux is just a fundamentally wrong premise.

A hard error feels appropriate to me here, unless there is a common case that I am not aware of.

[mgjarrett]

This is more copy/paste:

Based on the comment, it seems like the thinking goes that if an object without flux is passed in, you can just skip calculating the reaction rates on that object. You don't have to crash the whole run.

[keckler]

I guess I understand what is happening, but it feels outside of the purview of this method to not fail in this scenario. If I pass a block into a method called calcReactionRates() and it can't calculate the reaction rates, I would expect that method to fail, or at least tell me that it can't do it.

Just skipping feels like an error trap, to me. This is getting a bit outside the scope of your PR, because like you said you just copy/pasted. But maybe an opportunity for improvement in the future?

[mgjarrett]

I don't know whether we're ever actually hitting this exception. It might be good to remove it if possible, but that could probably be a separate PR where it is also removed from the original function I copied it out of.

[keckler]

Instead of looping over numberDensities and then checking that the number density is non-zero, would it make more sense to loop over the nuclides in xsNucDict? You've already done work to screen out isotopes with zero number density from there, so the number of isotopes in there is less and you don't have to do any further checking for zero values.

I might be misinterpreting this, I'm finding the logic somewhat complex.

[mgjarrett]

xsNucDict will have an entry for any nuclide that exists at a non-zero number density in any blockin theblockList`.

This line is checking whether the nuclide has a nonzero density in a particular block or other object.

[keckler]

True, maybe the check cannot be removed. But I still think that looping over the other list might make more sense, no? xsNucDict is shorter without loss of needed information.

[mgjarrett]

xsNucDict is not necessarily shorter than obj.getNumberDensities(). You could have a list of 500 blocks where 495 of them have 3 nuclides and 5 of them have 400 nuclides, and xsNucDict would have 400 nuclides. This is obviously an extreme situation that I've devised here that isn't realistic, but it's just hard to say that one of these lists is always going to be shorter than the other one.

If you refactored, it would be something like:

for nucName in xsNucDict:
    numberDensity = numberDensities.get(nucName, 0.0)
    if numberDensity == 0.0:
        continue

I think since you always have to get the dict value from numberDensities, it makes sense to iterate over numberDensities.items() instead of iterating over the keys and then doing numberDensities.get(key, 0.0). In general, in a group of blocks, if any of the blocks have a non-zero number density for a given nucName, then you'd have to iterate through that nuclide for every block and run numberDensities.get(nucName, 0.0) to check if that nuclide is there.

[keckler]

With those last couple comments, make any changes if you so desire. But I am good to approve this as-is. Thank you for this work, it will be a big improvement!

[john-science]

Approved, barring one change.