Move cold height expansion to bp

[onufer]

Description

---

Checklist

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

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

• The release notes (location doc/release/0.X.rst) are up-to-date with any bug fixes or new features.
• The documentation is still up-to-date in the doc folder.
• The dependencies are still up-to-date in setup.py.

[albeanth]

I see what you're getting at here. It makes sense. Let's do the expansion on the assemblies at the blueprints level as opposed to every single assembly in the core. For both standard ARMI runs and snapshot/restart runs with loadState, this will be faster and have the same outcome.

With some testing/cleanup etc I think it's a good idea. As part of the PR I think it would be cool to quantify the speedup too.

[onufer]

Load state will be the same since we use what’s in the db for that and only expand blueprints assems with load state (both before and after)

but o=armi.init will be much faster

[onufer]

The new implementation should have the exact same output/methodology as the old, but the implementation is simpler and faster, which is nice.

[john-science]

Hmmm

So, the primary blueprint code knows about the Axial Expansion?

But two-thirds of the projects that use ARMI, and half the ones at TerraPower, don't even have axial assemblies.

So, how does this code work for, say, a molten chloride reactor?

I ask for two reasons:

1. ARMI has to work for all reactors, not just one.
2. Axial Expansion is very slow, and for sure we don't want to instantiate all that unless we have to.

[onufer]

If you look at the code it’s only called if input heights considered cold

Both before and after this change the same calls are made by the time o=armi.init is done running. o=armi.init is about the minimal thing you can do and the new code will be just as fast or faster for those circumstances (because its producing the correct assemblies before the deepcopy, rather than deepcopying and then fixing them all after....

[john-science]

That's a fair point. Okay, so we aren't breaking non-axial-type reactors.

But we are importing something that is specific to a TYPE of reactor (albeit a common one) into a part of the code that (right now) reads YAML files and builds some math.

It just feels like something that should be in assemblyBlueprint.py. But, I assume, it was just hard to work into that module?

[onufer]

you need to know the meshes of each of the individual assemblies to go through the processes since they all have their indices mapped to the reference assembly (see a.makeAxialSnapList(referenceAssembly)). I dont see anywhere in that module where there are the full set of assemblies. The nice thing about doing it here is that the blueprints assemblies will exists for the shortest possible time ad the incorrect dimensions/masses since they are fixed right after construction (line 322)

[onufer]

if you want, right after the area consistancy check i could move all the new code to
axialExpansionChanger

and have it be

if not cs["detailedAxialExpansion"]:
    axialExpansionChanger.makeAssembliesSnapToUniformMesh(self.assemblies.values(), cs["nonUniformAssemFlags"])
if not cs["inputHeightsConsideredHot"]:
    axialExpansionChanger.expandColdDimsToHot(self.assemblies.values(),  cs["detailedAxialExpansion"])

Fundamentally though, blueprints should be responsible for producing assemblies with correct masses and elevations. It can certainly call someone else to do the work though

[john-science]

1. I see the problem you're trying to solve.
2. I love that you moved the axial expansion logic into that module.
3. Taking a step back, I think blueprints/__init__.py already has many direct references to assemblies at the top level. So I think you're not causing a design flaw so much as stuck within the system we created.
4. Let's explore your solution. The worst case is that each special type of reactor needs a bunch of these if cs["settingName"] statements, which point to some non-blueprint method. Unless that list gets long, that's probably going to look a little ugly, but not be heart-breakingly bad.
5. Sure, after moving the logic to the axial expansion module, I'm convinced.

Sorry, I'm not trying to be a bother. I have just learned by hard experience that improving something at the start can save a TON of time spent on technical debt later. I keep meaning to give a talk on that.

[onufer]

Great. thanks for clarifying @john-science. I genuinely don't think this list of things will grow considerably... I would recommend describing molten salt reactors and TRISO-based fuel as "non-pin-based" or "non-assembly-based" as opposed to "non-axial-type reactors".

Molten salt reactors still typically care about axial expansion of their vessel and the impact of that on reactivity (I spent a couple years on a molten salt project). Its just that the framework currently doesn't support it. Its not a trivial feature though and its a bit different from a fuel pin expanding. It took a couple hundred hours for us to get cold to hot dims working for pin-based-reactors. I am sure that if we had the capability to input cold vessel dims and have the framework expand the vessel to hot, molten salt reactors would want to use the capability. Its just we don't have it now, so those folks tend to continue to use hot axial heights.

If/when we have this capability, I would image we would change the call to expandColdDimsToHot to determine a different changer to use for non-pin based reactors rather than having another if statement here in blueprints.

Sure, after moving the logic to the axial expansion module, I'm convinced. what more do you need to approve the PR?

[john-science]

I would recommend describing molten salt reactors and TRISO-based fuel as "non-pin-based" or "non-assembly-based" as opposed to "non-axial-type reactors".

Thanks. And, always, I am interested in learning more about reactor design/analysis. Sorry, my graduate degrees are in physics, but not the right kind. I'm happy to learn.

what more do you need to approve the PR?

I added one little comment about a docstring, but I'm good otherwise. The logic looks sound, and I like the new layout better.

Thanks, man!

[john-science]

I'm not convinced the axial expansion logic belongs in the blueprints.

What about non-axial reactors? Tony and I worked over a couple of pull requests to pull all references to axial expansion OUT of the blueprints, and just have references to the logic.

What is the benefit of having our default blueprint logic aware of axial expansion? (Please note that two thirds of the projects most heavily using ARMI don't have axial layouts or assemblies. And even at TerraPower, one of our two reactors won't ever need to use ANY axial expansion logic.

Thoughts?

[onufer]

@john-science happy to move this stuff else where and have a 1 line call to hit here.

[onufer]

@john-science I absolutely think calls should to happen here rather than processCoreLoading. What's currently happening in processCoreLoading is

1. if its a db load just expand the blueprints since heights are params and correct
2. if its a first initialization expand both blueprints and assemblies in the reactor. BUT the only reason we have to expand the assems in the reactor is we failed to expand the blueprints before we deep-copied them to populate the reactor. Its a tremendous waste to deepcopy a whole reactor of assemblies that have incorrect masses and axial heights. The reason that we got here is there was a bit of debate about what he blueprints should store. what we landed on is that it should store axially expanded assemblies and it should be the same at each statepoint since its not in the database. therefore doing it here is the least expensive/most clear.

I think most of the logic is out of the blueprints... right now we are just checking some settings and calling outside methods/functions that update the state, but i would be happy to move the remainder out as well and have it be a 1 line call as shown in the comment..

Where would you want this stuff to live?

[onufer]

I do want the call to be front this location though so that the assemblies in blueprints exist at the wrong dimensions and densities as short a time as possible.

[onufer]

@john-science, I moved as much of the logic as possible out of blueprints, and added comments to hopefully communicate better what is happening and why its needed useful.

If you have more comments could you please be specific about what you want done? I feel like i am guessing/stabbing in the dark here... I could have a better chance of addressing the concern adequately if the comments were a little more specific/actionable. Thanks!

[john-science]

Great.

This solves a problem, and does it more by reorganizing already-tested code, than building things from scratch. The testing seems fine, and I love that this PR has one goal, and not 12.

Thanks for that.

[onufer]

@albeanth here is what time looks like on small test reactor....
before PR
no expand = 24.5 sec
expand cold to hot = 49.5 sec

after PR
no expand = 21.2 sec
expand cold to hot = 25.9 sec

some reactors take more like 5 minutes to load, so folks should notice a difference.

import armi

armi.configure()

from armi.reactor.tests import test_reactors
import timeit

from armi import context

context.Mode.setMode(context.Mode.BATCH)  # disable yes/no CLI

hot = timeit.timeit(
    'test_reactors.loadTestReactor(customSettings={"inputHeightsConsideredHot": True})',
    setup="from armi.reactor.tests import test_reactors",
    number=10,
)

cold = timeit.timeit(
    'test_reactors.loadTestReactor(customSettings={"inputHeightsConsideredHot": False})',
    setup="from armi.reactor.tests import test_reactors",
    number=10,
)

print(hot, cold)