Fixing sub-block grids #1947

john-science · 2024-10-10T23:46:22Z

What is the change?

It appears there was a bug where Components inside a Block (like fuel pins) would have the wrong location if they were on a Hex grid that was corners-side up. The pins would be incorrectly rotated to the "corners up" grid rotation, even they should be flats-up.

The change I made to fix the issue:

Move adding a spatial grid to Components in a Block until the Assembly containing those Blocks is added to the Core or SFP
calculate "Is this a corners up grid?" and pass that into the method that adds the spatial grid to the Blocks (autoCreateSpatialGrid())

Why is the change being made?

Because fixing bugs is good.

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.

john-science · 2024-10-10T23:48:26Z

I have proof that this WAS a bug, but this PR fixes it. I added a bunch of unit tests in this PR, but they all pass here, however, if you copy one of the new ones to ARMI main, it fails:

__________________________________ TestHexBlockOrientation.test_validateFlatsUp __________________________________ 

Traceback (most recent call last):
  File "Python3.9.7\lib\unittest\case.py", line 59, in testPartExecutor
    yield
  File "Python3.9.7\lib\unittest\case.py", line 592, in run
    self._callTestMethod(testMethod)
  File "Python3.9.7\lib\unittest\case.py", line 550, in _callTestMethod
    method()
  File "armi\armi\reactor\tests\test_blocks.py", line 2281, in test_validateFlatsUp
    self.assertFalse(b.spatialGrid.cornersUp)
  File "Python3.9.7\lib\unittest\case.py", line 674, in assertFalse
    raise self.failureException(msg)
AssertionError: True is not false

mgjarrett · 2024-10-11T16:07:29Z

I haven't reviewed this, but I skimmed it and I think this solution is great! I had stopped working on #1648 because I ran into the exact problem that this PR addresses and I didn't know how we wanted to resolve it. This looks good to me 👍

drewj-tp

Thanks for digging into this @john-science. I think this is the most feasible way for us to solve the problem.

I'm going to provide some context as to why we need to loop over every block in every assembly during Core.add to solve this problem.

When we make a block from blueprints, we have not fully created the assembly, let alone the grid for that assembly.
It is possible to use the same assembly design in two different grids, so we can't set the orientation in the "cached" assembly stored on the blueprint object during constructing assemblies
We don't really know the orientation of a block in an assembly until that assembly is fully realized and added to a thing (core, sfp, etc.)

armi/reactor/spentFuelPool.py

armi/reactor/tests/test_blocks.py

drewj-tp · 2024-10-11T19:00:38Z

armi/reactor/assemblies.py

+    def orientBlocks(self, cornersUp):
+        """Add special grids to the blocks inside this Assembly, respecting their orientation."""
+        for b in self:
+            if b.spatialGrid is None:
+                try:
+                    b.autoCreateSpatialGrids(cornersUp)
+                except (ValueError, NotImplementedError) as e:
+                    runLog.warning(str(e), single=True)
+


Should this live on the HexAssembly because we have a cornerUp flag that is hex specific?

All assemblies have a .autoCreateSpatialGrids() method though, right?

Blocks yes, but the cornersUp switch is hex specific. What does this mean for a cartesian grid? I get that it's good OOP practice to have the same signatures for overridden methods, but this feels in the vein of "everything is hexagonal in ARMI"

armi/reactor/blueprints/blockBlueprint.py

drewj-tp · 2024-10-11T19:05:43Z

armi/reactor/blocks.py

        if self.spatialGrid is None:
            self.spatialGrid = grid


The caller for this method in Assembly.orientBlocks only calls this if HexBlock.spatialGrid is None. Do we need to have this check here?

This is a public method. People can call it multiple times, any time they want, downstream in their plugins.

So, this method would cause havoc if we let people call it multiple times in a row.

(I am considering moving this check (if self.spatialGrid is None) up to the top of the method in a different PR, by the way. But we have like 6 unit tests designed specifically for this check to work this way that will break. So I didn't want to muddy the waters and make this PR less clear for you to read. So I'll do it as it's own stand-alone PR.)

armi/reactor/blocks.py

armi/reactor/grids/tests/test_grids.py

Co-authored-by: Drew Johnson <anjohnson@terrapower.com>

drewj-tp · 2024-10-14T19:01:47Z

Talked with @john-science and I wanted to clarify something that I missed in the initial review

The pins would be incorrectly rotated to the "corners up" grid rotation, even for flats-up grids

A hexagonal block on a flats-up core grid should have a corners up pin grid if it intends to tightly pack pins into the block. If you look at the problem plot from #1421

it looks like the pins are in a flats up grid because they form the shape of a flats up hexagon. But! The individual pins are on a corners up grid if you draw a tiny hexagon around each lattice site. Or the ability to walk from neighbor to neighbor in the x-direction does not require moving in the y-direction.

into sub_block_grids

drewj-tp

My main concern is that Block.spatialGrid should be the grid on which its children live. I added some suggestions to help communicate this and I hope I caught enough locations so accepting the changes will still let unit tests pass.

But I believe our desired outcome is

b.spatialGrid.cornersUp !=  b.parent.parent.spatialGrid.cornersUp

drewj-tp · 2024-10-16T15:31:20Z

armi/reactor/blocks.py

        grid = grids.HexGrid.fromPitch(
-            self.getPinPitch(cold=True), numRings=0, cornersUp=cornersUp
+            self.getPinPitch(cold=True),
+            numRings=0,
+            armiObject=self,
+            cornersUp=cornersUp,
+        )
+
+        # build the sub-grid, with opposite orientation to the block
+        subGrid = grids.HexGrid.fromPitch(
+            self.getPinPitch(cold=True),
+            numRings=0,
+            armiObject=self,
+            cornersUp=not cornersUp,


This is a deviation from how ARMI has previously reported the pin locations. On main, the following takes place

>>> r = armi.init(fName="armi/tests/tutorials/anl-afci-177.yaml").r >>> a = r.core.getFirstAssembly(Flags.FUEL) >>> b = a.getFirstBlock(Flags.FUEL) >>> fuel = b.getChildrenWithFlags(Flags.FUEL)[0] >>> fuel.spatialLocator.grid is b.spatialGrid True

The docs for Composite.spatialGrid and Composite.spatialLocator could be better, but they allude to this fact

armi/armi/reactor/composites.py

Lines 322 to 325 in 01b0ade

spatialGrid : grids.Grid

The spatial grid that this object contains

spatialLocator : grids.LocationBase

The location of this object in its parent grid, or global space

So Composite.spatialGrid is the sub-grid, or the grid on which the children live. Core.spatialGrid is the grid for all the assemblies, Assembly.spatialGrid for blocks, Block.spatialGrid for components.

I think the negation needs to go up a level higher:

HexBlock.autoCreateSpatialGrid makes the HexBlock.spatialGrid in the orientation prescribed by cornersUp

All the child components live on the grid defined by HexBlock.spatialGrid

When we add the HexAssembly to a Core.spatialGrid (or SFP) we need to invert the orientation with e.g.,

def add(self, hexAssem): blockGridCornersUp = not self.spatialGrid.cornersUp for b in hexAssem: b.autoCreateSpatialGrid(..., cornersUp=blockGridCornersUp)

drewj-tp · 2024-10-16T15:33:18Z

armi/reactor/grids/grid.py

+    @property
+    def cornersUp(self) -> bool:
+        return False
+


CartesianGrid.cornersUp doesn't make sense to me. This seems like putting hex specific logic onto the base class just to unify the API.

drewj-tp · 2024-10-16T15:34:01Z

armi/reactor/assemblies.py

+    def orientBlocks(self, cornersUp):
+        """Add special grids to the blocks inside this Assembly, respecting their orientation."""
+        for b in self:
+            if b.spatialGrid is None:
+                try:
+                    b.autoCreateSpatialGrids(cornersUp)
+                except (ValueError, NotImplementedError) as e:
+                    runLog.warning(str(e), single=True)
+


Blocks yes, but the cornersUp switch is hex specific. What does this mean for a cartesian grid? I get that it's good OOP practice to have the same signatures for overridden methods, but this feels in the vein of "everything is hexagonal in ARMI"

drewj-tp · 2024-10-16T15:36:41Z

armi/reactor/blocks.py

-    def autoCreateSpatialGrids(self):
+    def autoCreateSpatialGrids(self, cornersUp=True):


Same reservations about putting cornersUp switch in the generic Block.autoCreateSpatialGrids

drewj-tp · 2024-10-16T15:45:55Z

armi/reactor/grids/tests/test_grids.py

+        self.assertLess(maxY, 1)
+        self.assertGreater(minY, -1)


Recommend a stronger check for asserting max and min y locations are +/- pitch * cos(60)

drewj-tp · 2024-10-16T15:47:35Z

armi/reactor/cores.py

        self.assembliesByName[aName] = a
        for b in a:
            self.blocksByName[b.getName()] = b

        if self.geomType == geometry.GeomType.HEX:
+            cornerUp = self.spatialGrid.cornersUp
+            a.orientBlocks(cornersUp=cornerUp)


Block.spatialGrid should be the opposite orientation of the grid on which its parent assembly lies

Suggested change

a.orientBlocks(cornersUp=cornerUp)

a.orientBlocks(cornersUp=not cornerUp)

drewj-tp · 2024-10-16T15:48:36Z

armi/reactor/tests/test_blocks.py

+
+        # grab a pinned fuel block, and verify it is corners up
+        b = r.core.getFirstBlock(Flags.FUEL)
+        self.assertTrue(b.spatialGrid.cornersUp)


This test reactor has a corners up core grid so Block.spatialGrid should be flats up

Suggested change

self.assertTrue(b.spatialGrid.cornersUp)

self.assertFalse(b.spatialGrid.cornersUp)

drewj-tp · 2024-10-16T15:49:27Z

armi/reactor/tests/test_blocks.py

+        b = r.core.getFirstBlock(Flags.FUEL)
+        self.assertTrue(b.spatialGrid.cornersUp)
+
+        # for a flats up sub-grid, the hex centroids should stretch more in Y than X


Suggested change

# for a flats up sub-grid, the hex centroids should stretch more in Y than X

# for a flats up block-grid, the hex centroids should stretch more in Y than X

drewj-tp · 2024-10-16T15:50:04Z

armi/reactor/tests/test_blocks.py

+
+        # grab a pinned fuel block, and verify it is flats up
+        b = r.core.getFirstBlock(Flags.FUEL)
+        self.assertFalse(b.spatialGrid.cornersUp)


Block.spatialGrid in a flats up core should be corners up

Suggested change

self.assertFalse(b.spatialGrid.cornersUp)

self.assertTrue(b.spatialGrid.cornersUp)

drewj-tp · 2024-10-16T15:50:11Z

armi/reactor/tests/test_blocks.py

+        b = r.core.getFirstBlock(Flags.FUEL)
+        self.assertFalse(b.spatialGrid.cornersUp)
+
+        # for a corners up sub-grid, the hex centroids should stretch more in X than Y


Suggested change

# for a corners up sub-grid, the hex centroids should stretch more in X than Y

# for a corners up block-grid, the hex centroids should stretch more in X than Y

john-science · 2024-10-16T22:01:15Z

@drewj-tp I need to disambiguate. You say:

My main concern is that Block.spatialGrid should be the grid on which its children live.

But below that you have 4 concerns I see:

You want b.spatialGrid.cornersUp != b.parent.parent.spatialGrid.cornersUp
You want b.spatialGrid.cornersUp == list(b.getChildren())[0].spatialGrid.cornersUp
You don't want the base Grid class to have a .cornersUp() method.
You don't want to pass cornersUp into autoCreateSpatialGrid.

Do I have that right?

drewj-tp · 2024-10-17T15:12:57Z

@drewj-tp I need to disambiguate. You say:

My main concern is that Block.spatialGrid should be the grid on which its children live.

But below that you have 4 concerns I see:

Correct. I should be clearer on concern level and preference.

You want b.spatialGrid.cornersUp != b.parent.parent.spatialGrid.cornersUp

If we are doing HexBlock.autoCreateSpatialGrid yes, this is a must have.

Under the assumption the user wants to pack some number of pin-like objects into a hexagon, the grid of the hexagonal lattice should be 30-degrees rotated from the orientation of the block.

A user should still be allowed to explicitly create a flats up pin grid in a flats up core grid via blueprints.

You want b.spatialGrid.cornersUp == list(b.getChildren())[0].spatialGrid.cornersUp

Almost. Composite.spatialGrid indicates this attribute is the grid for children of the composite. Core.spatialGrid is the grid on which assemblies live. The Composite.spatialLocator tells us where this object lives in it's parents grid. So

all(child.spatialLocator.grid is parent.spatialGrid for child in parent) == True

That is how I believe ARMI does it already and it makes sense to me. This is also a must have in that I think deviations are a backwards incompatible change that I don't believe is warranted.

You don't want the base Grid class to have a .cornersUp() method.

Want yes. Must have, no. We already have a lot of hex specific stuff on base classes. Why stop now.

You don't want to pass cornersUp into autoCreateSpatialGrid.

Want, yes. Must have, no. We have a lot of hex specific stuff in methods that should be generic. Why stop now.

john-science added 3 commits October 10, 2024 15:56

Save point

0a3764c

linting

f19ea49

cleanup

b79e2e7

john-science added the bug Something is wrong: Highest Priority label Oct 10, 2024

john-science requested a review from drewj-tp October 10, 2024 23:46

Merge branch 'main' into sub_block_grids

ac34bb3

john-science mentioned this pull request Oct 11, 2024

Fixing HexBlock rotation in plotBlockDiagram #1926

Draft

7 tasks

Adding release note

b244c8a

john-science mentioned this pull request Oct 11, 2024

HexBlock.rotate updates child spatial locators #1943

Draft

8 tasks

drewj-tp requested changes Oct 11, 2024

View reviewed changes

john-science and others added 6 commits October 11, 2024 13:26

Apply suggestions from code review

55c6715

Co-authored-by: Drew Johnson <anjohnson@terrapower.com>

Responding to comments

dc73342

responding to reviewer: test docstrings

1942ed1

Removed unused setting autoGenerateBlockGrids

3facbc8

Anchroring grid to block - per reviewer

37dd62e

Responding to comments - test refactor

0d55251

john-science requested a review from drewj-tp October 11, 2024 22:04

Merge branch 'main' into sub_block_grids

2f8a350

john-science and others added 5 commits October 14, 2024 13:42

Merge branch 'main' into sub_block_grids

349e406

On a hex lattice, sub-hex grids are rotated

7d8ab19

Merge branch 'main' into sub_block_grids

6141cc7

Making unit tests more stable

e8182dd

Merge branch 'sub_block_grids' of https://www.github.com/terrapower/armi

4f8242a

into sub_block_grids

drewj-tp requested changes Oct 16, 2024

View reviewed changes

Blocks are on same grid as Core

86516c7

john-science marked this pull request as draft October 16, 2024 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing sub-block grids #1947

Fixing sub-block grids #1947

john-science commented Oct 10, 2024 •

edited

Loading

john-science commented Oct 10, 2024

mgjarrett commented Oct 11, 2024

drewj-tp left a comment

drewj-tp Oct 11, 2024

john-science Oct 11, 2024

drewj-tp Oct 16, 2024

drewj-tp Oct 11, 2024

john-science Oct 11, 2024

drewj-tp commented Oct 14, 2024

drewj-tp left a comment

drewj-tp Oct 16, 2024

drewj-tp Oct 16, 2024

drewj-tp Oct 16, 2024

drewj-tp Oct 16, 2024

drewj-tp Oct 16, 2024

drewj-tp Oct 16, 2024

drewj-tp Oct 16, 2024

drewj-tp Oct 16, 2024

drewj-tp Oct 16, 2024

drewj-tp Oct 16, 2024

john-science commented Oct 16, 2024

drewj-tp commented Oct 17, 2024

	spatialGrid : grids.Grid
	The spatial grid that this object contains
	spatialLocator : grids.LocationBase
	The location of this object in its parent grid, or global space

		def autoCreateSpatialGrids(self):
		def autoCreateSpatialGrids(self, cornersUp=True):

	a.orientBlocks(cornersUp=cornerUp)
	a.orientBlocks(cornersUp=not cornerUp)

	self.assertTrue(b.spatialGrid.cornersUp)
	self.assertFalse(b.spatialGrid.cornersUp)

	# for a flats up sub-grid, the hex centroids should stretch more in Y than X
	# for a flats up block-grid, the hex centroids should stretch more in Y than X

	# for a corners up sub-grid, the hex centroids should stretch more in X than Y
	# for a corners up block-grid, the hex centroids should stretch more in X than Y

Fixing sub-block grids #1947

Are you sure you want to change the base?

Fixing sub-block grids #1947

Conversation

john-science commented Oct 10, 2024 • edited Loading

What is the change?

Why is the change being made?

Checklist

john-science commented Oct 10, 2024

mgjarrett commented Oct 11, 2024

drewj-tp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewj-tp commented Oct 14, 2024

drewj-tp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-science commented Oct 16, 2024

drewj-tp commented Oct 17, 2024

john-science commented Oct 10, 2024 •

edited

Loading