-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixing sub-block grids #1947
Fixing sub-block grids #1947
Conversation
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 __________________________________ 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 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
Co-authored-by: Drew Johnson <anjohnson@terrapower.com>
Talked with @john-science and I wanted to clarify something that I missed in the initial review
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 I need to disambiguate. You say:
But below that you have 4 concerns I see:
Do I have that right? |
@drewj-tp Progress
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 That being said, I understand why you don't like the design choice to pass the information down. I'll look at it again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pins on the block are in the incorrect orientation still, due to the sub-block flipped grid.
Quick plotter
"""Plot pins in an assembly in a reactor"""
import math
import pathlib
from matplotlib import pyplot, patches
import armi
from armi.reactor.flags import Flags
from armi.reactor.blocks import HexBlock
from armi.reactor.components import Circle, Hexagon
from armi.reactor.tests.test_reactors import TEST_ROOT, loadTestReactor
def _plotPins(c: Circle, ax: pyplot.Axes):
loc = c.spatialLocator
radius = 0.5 * c.getDimension("od")
for l in loc:
xy = l.getLocalCoordinates()[:2]
patch = patches.Circle(xy, radius=radius)
ax.add_patch(patch)
def _plotDuct(c: Hexagon, ax: pyplot.Axes, cornersUp: bool):
patch = patches.RegularPolygon(
xy=(0, 0),
numVertices=6,
radius=c.getDimension("op") / 2,
facecolor="w",
edgecolor="k",
orientation=0 if cornersUp else math.radians(30),
)
ax.add_patch(patch)
def plotGeometry(b: HexBlock, ax: pyplot.Axes, cornersUpBlock: bool):
d = b.getChildrenWithFlags(Flags.DUCT)[0]
_plotDuct(d, ax, cornersUpBlock)
f = b.getChildrenWithFlags(Flags.FUEL)[0]
_plotPins(f, ax)
window = round(d.getDimension("op") * 0.525)
ax.set(
xlim=(-window, window),
ylim=(-window, window),
)
if __name__ == "__main__":
armi.configure()
_, r = loadTestReactor(
pathlib.Path(TEST_ROOT) / "smallestTestReactor",
inputFileName="armiRunSmallest.yaml",
)
b = r.core.getFirstBlock(Flags.FUEL)
plotGeometry(
b, pyplot.gca(), cornersUpBlock=r.core.spatialGrid.cornersUp,
)
pyplot.savefig("hex_pins.png")
pyplot.show()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @john-science! Nothing more from me 🖖 🚀
This reverts commit 6b356b3.
Merge remote-tracking branch 'zprince/component_flux_up' into drewj/bu-rotate-with-pin-dep * zprince/component_flux_up: start rm'img things Fixing HexBlock rotation in plotBlockDiagram (#1926) Removing defunct import (#1986) Fixing wetted perimeter for hex inner ducts (#1985) Fixing sub-block grids (#1947) make history tracker respect control assemblies comment out print statements add defaults to pin level params add c.p.puFrac calc, move b.getPuMoles up the composite tree, add a pu frac comp params add c.p.molesHmBOL and populate it Adding component method to retrieve pin fluxes Revert "Moving pin flux parameters to component level" add pinPercentBu component param Moving pin flux parameters to component level
Blocks do not always have a spatialGrid from Blueprints, but some Blocks can have their | ||
spatialGrids inferred based on the multiplicty of their components. This would add the | ||
ability to create a spatialGrid for a Block and give its children the corresponding | ||
spatialLocators if certain conditions are met. | ||
|
||
Parameters | ||
---------- | ||
systemSpatialGrid : Grid, optional | ||
Spatial Grid of the system-level parent of this Assembly that contains this Block. | ||
|
||
Raises | ||
------ | ||
ValueError | ||
If the multiplicities of the block are not only 1 or N or if generated ringNumber leads | ||
to more positions than necessary. | ||
""" | ||
raise NotImplementedError() | ||
if self.spatialGrid is None: | ||
self.spatialGrid = systemSpatialGrid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@john-science Am I mistaken, or is the docstring for this method pretty wrong? A couple points seem incorrect to me:
- The method does not raise any errors, so the
Raises
section of the docstring seems wrong - This method does not appear to have the "ability to create a spatialGrid" as the docstring states
Maybe this docstring was intended for the child class?
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:
autoCreateSpatialGrid()
)Why is the change being made?
Because fixing bugs is good.
Checklist
doc
folder.pyproject.toml
.