Stop abusing Blueprints._prepConstruction or make it public

[drewj-tp]

Blueprints._prepConstruction does a lot. Its docstring even says as much

https://github.com/terrapower/armi/blob/f318392fd0804adc3a579c5edf6fca1df0e40d0f/armi/reactor/blueprints/__init__.py#L276-L278

Not only does it construct the design of each individual assembly type, it

1. Does processing on nuclides found in the blueprints.
2. Checks the area of assemblies.
3. Conditionally configures the axial snap mesh.
4. Conditionally performs initial thermal axial expansion on assemblies.
5. Calls the afterConstructionOfAssemblies plugin hook.

That's a lot! The value there is clear, as all those things must be done in order to make sure the assembly designs are ready to be used.

While I was working on the axial expansion work (#1843), I noticed that this private method is used directly. Which raises some hairs on the back of my neck because, 1) it points out this contradiction in its own docstring

https://github.com/terrapower/armi/blob/f318392fd0804adc3a579c5edf6fca1df0e40d0f/armi/reactor/blueprints/__init__.py#L288

and 2) I am of the belief that non-public methods (single or double leading underscore) should not be called by anything that is not the owning class. Yes, everything in Python is public, somethings just harder to access. But this public/protected/private distinction also allows developers some wiggle room.

If a method or attribute was truly protected, I have more freedom to refactor and break it up or think around that method. Maybe the four things it does don't all need to happen at once can be moved around. But if it's functionally public, it's a heavier lift.

Resolution

I propose that we rethink Blueprints._prepConstruction and determine how to either

1. stop misusing (in my opinion) a protected method, or
2. what functionality callers are expecting and make that public.

I've collected the cases where we use this method. Thankfully the majority of them are in testing. I presume that most are needed because we want to build assemblies without spinning up entire reactors from blueprints, but the blueprints class requires the assembly to be processed in _prepConstruction.

Non-test modules

https://github.com/terrapower/armi/blob/f318392fd0804adc3a579c5edf6fca1df0e40d0f/armi/cases/suiteBuilder.py#L168

https://github.com/terrapower/armi/blob/f318392fd0804adc3a579c5edf6fca1df0e40d0f/armi/reactor/reactors.py#L2471

Test modules

https://github.com/terrapower/armi/blob/f318392fd0804adc3a579c5edf6fca1df0e40d0f/armi/reactor/tests/test_blocks.py#L316

https://github.com/terrapower/armi/blob/f318392fd0804adc3a579c5edf6fca1df0e40d0f/armi/reactor/tests/test_assemblies.py#L597

https://github.com/terrapower/armi/blob/f318392fd0804adc3a579c5edf6fca1df0e40d0f/armi/reactor/tests/test_assemblies.py#L1442

https://github.com/terrapower/armi/blob/f318392fd0804adc3a579c5edf6fca1df0e40d0f/armi/reactor/blueprints/tests/test_blueprints.py#L63

https://github.com/terrapower/armi/blob/f318392fd0804adc3a579c5edf6fca1df0e40d0f/armi/reactor/blueprints/tests/test_blueprints.py#L306

https://github.com/terrapower/armi/blob/f318392fd0804adc3a579c5edf6fca1df0e40d0f/armi/materials/tests/test_materials.py#L2013

https://github.com/terrapower/armi/blob/f318392fd0804adc3a579c5edf6fca1df0e40d0f/armi/cases/inputModifiers/tests/test_pinTypeInputModifiers.py#L31

https://github.com/terrapower/armi/blob/f318392fd0804adc3a579c5edf6fca1df0e40d0f/armi/reactor/blueprints/tests/test_materialModifications.py#L62

https://github.com/terrapower/armi/blob/f318392fd0804adc3a579c5edf6fca1df0e40d0f/armi/cases/inputModifiers/tests/test_inputModifiers.py#L131

https://github.com/terrapower/armi/blob/f318392fd0804adc3a579c5edf6fca1df0e40d0f/armi/reactor/blueprints/tests/test_assemblyBlueprints.py#L182

https://github.com/terrapower/armi/blob/f318392fd0804adc3a579c5edf6fca1df0e40d0f/armi/reactor/blueprints/tests/test_blockBlueprints.py#L265

[john-science]

I'm not worried about the times when this pops up in testing.

Unit tests should be able to test private methods. Just because logic is internal to a class doesn't mean it's not complicated enough, or important enough, to warrant testing.

So, from my perspective, this only pops up once in a non-test environment. Still, that IS interesting.

[drewj-tp]

Unit tests should be able to test private methods

Agree to a point. A unit test on the Blueprints object using and testing protected methods on that object> I can get behind that.

But at test for a materials object needing a private method from `Blueprints?

https://github.com/terrapower/armi/blob/f318392fd0804adc3a579c5edf6fca1df0e40d0f/armi/materials/tests/test_materials.py#L2010-L2014

I think that's stretching the domain of applicability.

I think nearly all of these could be mitigated if we have a Blueprints.buildAssembly method because that's what the majority of these tests need. After some brief skimming at least.

this only pops up once in a non-test environment

Twice

https://github.com/terrapower/armi/blob/f318392fd0804adc3a579c5edf6fca1df0e40d0f/armi/cases/suiteBuilder.py#L168

and

https://github.com/terrapower/armi/blob/f318392fd0804adc3a579c5edf6fca1df0e40d0f/armi/reactor/reactors.py#L2471

[john-science]

I agree, if this is needed to be public in a non-test setting, we should probably make it public. Sure.

[drewj-tp]

FWIW I have been able to have identical success replacing patterns of

bp._prepConstruction(cs)
assem = bp.assemblies[assem_name]

with

assem = bp.constructAssembly(cs, name=assem_name)

The tradeoff is bp.constructAssembly does a deepcopy of an assembly and calls Assembly.makeUnique prior to returning the new assembly. Those may or may not be important for testing but are certainly important for production analysis