Improving getOptimalAssemblyOrientation

[drewj-tp]

What is the change?

The algorithm goes as follows

1. Get all the pin powers and IndexLocations from the block at the previous location (previous timestep)
2. Obtain the IndexLocation of the pin with the highest burnup
3. For each possible rotation,
   • Find the new location with HexGrid.rotateIndex
   • Find the previous power at that location
4. Return the rotation with the lowest previous power

This algorithm assumes a few things.

1. len(pinLocations) == len(pinPowers) in both cases. This may make sense, but we've found some cases where this assumption breaks. Not even edge cases, like the C5G7 LWR benchmark.
2. Your assembly has at least 60 degree symmetry of fuel pins and powers. This means if we find a fuel pin with high burnup and rotate it 60 degrees, there should be another fuel pin at that lattice site. This is mostly a safe assumption since many hexagonal reactors have at least 60 degree symmetry of fuel pin layout. This assumption holds if you have a full hexagonal lattice of fuel pins as well.
3. Fuel pins in a have similar locations in aPrev. This is mostly a safe assumption in that most fuel assemblies have similar layouts so it's plausible that if a has a fuel pin at (1, 0, 0), so does aPrev.

Why is the change being made?

The getOptimalAssemblyOrientation used to rely on things that either don't exist or don't get updated anymore. Like Block.p.pinLocations. And it's testing was subpar: it just checked that the new rotation was different, not correct.

armi/armi/physics/fuelCycle/tests/test_assemblyRotationAlgorithms.py

Lines 30 to 50 in f1f3dbe

	class TestFuelHandlerMgmtTools(FuelHandlerTestHelper):
	def test_buReducingAssemblyRotation(self):
	fh = fuelHandlers.FuelHandler(self.o)
	hist = self.o.getInterface("history")
	newSettings = {CONF_ASSEM_ROTATION_STATIONARY: True}
	self.o.cs = self.o.cs.modified(newSettings=newSettings)
	assem = self.o.r.core.getFirstAssembly(Flags.FUEL)
	# apply dummy pin-level data to allow intelligent rotation
	for b in assem.getBlocks(Flags.FUEL):
	b.breakFuelComponentsIntoIndividuals()
	b.initializePinLocations()
	b.p.percentBuMaxPinLocation = 10
	b.p.percentBuMax = 5
	b.p.linPowByPin = list(reversed(range(b.getNumPins())))
	addSomeDetailAssemblies(hist, [assem])
	rotNum = b.getRotationNum()
	rotAlgos.buReducingAssemblyRotation(fh)
	self.assertNotEqual(b.getRotationNum(), rotNum)

Additionally, we now have a pathway for plugins defining pin-level burnup as Component.p.pinPercentBu over block-level parameters.

Closes #1921

Other noteworthy changes

Don't rotate assemblies until all assembly rotations have been determined

If we rotate an assembly that is then used to determine how a different assembly should be rotated, we've poisoned our check. We want to compare against the state of the assembly that was in the core when the power was produced.

Consider:

1. Assembly A is shuffled to a new location and then rotated.
2. Assembly B is shuffled to the previous location of assembly A.

The power profile that is used to determine the rotation of assembly B is based on a rotated assembly A. This is not ideal because we want to compare against the power profile assembly B is expected to experience. And by rotating assembly A before this check, we've introduced a bias into the check.

Now, the buReducingAssemblyRotation will store the number of rotations for each assembly first, then rotate all assemblies at once.

Additionally, any assembly that is found to have a zero rotation is not going to be rotated. Calling Assembly.rotate(0) is a no-op that we don't need to consider. These no-rotations will also be excluded from the counting of rotations.

Improved handling of fresh assemblies

a0bb7ee

In some testing, I found that freshly created assemblies caused problems with the burnup reducing rotation algorithm. They didn't have a useful previous location, in that their lastLocationLabel was LoadQueue since they had yet to be placed within the core. This caused problems trying to find the assembly that now exists where they used to be because the logic for breaking an assembly locator label like 001-003 breaks when that label is LoadQueue.

The fuel handler tests now work higher up on the FuelHandler.outage method that invokes the buReducingAssemblyRotation function.

In the case of the fresh fuel, we fake a case where an assembly is created fresh and dropped into the reactor without moving another assembly. FuelHandler.moved only really makes sense for N>1 entries if you're swapping or disharging assemblies. But, we want to show that we skip assemblies that are coming from outside the reactor.

Testing testing testing

Ensure that we get the correct rotation, not just a different rotation.

Adds a test where we involve three assemblies A -> B -> C mimicking a real outage event. This test is important to make sure we aren't rotating assemblies that we need later.

Heavy refactor to make the tests a bit easier to read (hopefully) by adding a few things

1. PinLocations enumeration to de-mystify some magic pin locations.
2. Helper methods for getting burnup vector with some known max location and pin power vector with known min locaiton
3. Comparison of the rotation called on an assembly with the integer number of rotations we know from the test

---

Personal TODO

• Remove debug statements once I've re-tested against new changes
• Release notes for new features and removed / renamed parameters

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.

[drewj-tp]

Reverted the dropped and replaced pin burnup parameters because they were more entangled than I thought, and they are a want not a need right now.

[john-science]

@drewj-tp Are you hoping this PR will make it into the (imminent) ARMI 0.5.0 release?

[drewj-tp]

@drewj-tp Are you hoping this PR will make it into the (imminent) ARMI 0.5.0 release?

Yes. It is my understanding that this is a need for the release based on user needs

[john-science]

Suggested change

	raise ValueError(msg)
	maxBuPinLocation = maxBurnupLocator(maxBuBlock)
	raise ValueError(msg)
	# 2. Obtain the ``IndexLocation`` of the pin with the highest burnup in the current assembly.
	maxBuPinLocation = maxBurnupLocator(maxBuBlock)

[john-science]

We don't have to do this exactly, but there is a lot of stuff happening in this function, so I am just trying to make it a touch easier to read.

Your call.

[drewj-tp]

If it's my call, I'm going to forgo this and similar requests.

I get readability through comments. I've also seen comments rot as the code changes while comments are not updated accordingly. So I'm inclined to have more verbose variable names and functions over comments. To hopefully help explain things.

maxBuPinLocation feels to me like a thing that tells us where the pin with the highest burnup can be found

It's very much a personal stance of mine that, if I'm left to decide, I'm going to lean on fewer comments but more descriptive docstrings and verbose variables/functions

[john-science]

Okay, that's fine.

That's not the M.O. for ARMI though. I don't particularly feel that docstrings are less likely to go out of date than comments. And without your docstring, this function's logic would be too complicated for someone to read at a glance. That's really my problem, I don't want some poor engineer to come to this function in 3 years and have to puzzle out this important logic on their own.

To that end, the docstring can stand as sufficient.

[john-science]

It looks good.

Thanks so much for all the hard work on the testing!

I have some thoughts for you, but none on the basic logic. That seems solid to me.