Fixing sub-block grids
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
- [x] This PR has only one purpose or idea.
- [x] Tests have been added/updated to verify any new/changed code.
- [x] The code style follows good practices.
- [x] The commit message(s) follow good practices.
- [x] The release notes have been updated if necessary.
- [x] The documentation is still up-to-date in the
docfolder. - [x] The dependencies are still up-to-date in
pyproject.toml.
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
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 👍
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.
@drewj-tp I need to disambiguate. You say:
My main concern is that
Block.spatialGridshould 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
Gridclass to have a.cornersUp()method. - You don't want to pass
cornersUpintoautoCreateSpatialGrid.
Do I have that right?
@drewj-tp I need to disambiguate. You say:
My main concern is that
Block.spatialGridshould 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
Gridclass 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
cornersUpintoautoCreateSpatialGrid.
Want, yes. Must have, no. We have a lot of hex specific stuff in methods that should be generic. Why stop now.
@drewj-tp Progress
- [x] You want b.spatialGrid.cornersUp != b.parent.parent.spatialGrid.cornersUp
- [x] 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.
So, the PR should "work" now. But I haven't hit your design choices. The problem is that we NEED to be able to pass information down from the Core or SpentFuelPool about the orientation of hex blocks, because self.parent isn't guaranteed to exist in all cases.
That being said, I understand why you don't like the design choice to pass the information down. I'll look at it again.