armi icon indicating copy to clipboard operation
armi copied to clipboard

Fixing sub-block grids

Open john-science opened this issue 1 year ago • 5 comments

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] The release notes have been updated if necessary.
  • [x] The documentation is still up-to-date in the doc folder.
  • [x] The dependencies are still up-to-date in pyproject.toml.

john-science avatar Oct 10 '24 23:10 john-science

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

john-science avatar Oct 10 '24 23:10 john-science

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 👍

mgjarrett avatar Oct 11 '24 16:10 mgjarrett

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 avatar Oct 14 '24 19:10 drewj-tp

@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:

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

Do I have that right?

john-science avatar Oct 16 '24 22:10 john-science

@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.

  1. 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.

image

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

  1. 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.

  1. 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.

  1. 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.

drewj-tp avatar Oct 17 '24 15:10 drewj-tp

@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.

john-science avatar Oct 22 '24 18:10 john-science