Skip to content
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

Merged
merged 38 commits into from
Oct 29, 2024
Merged

Fixing sub-block grids #1947

merged 38 commits into from
Oct 29, 2024

Conversation

john-science
Copy link
Member

@john-science john-science commented Oct 10, 2024

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

  • 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 john-science added the bug Something is wrong: Highest Priority label Oct 10, 2024
@john-science john-science requested a review from drewj-tp October 10, 2024 23:46
@john-science
Copy link
Member Author

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
Copy link
Contributor

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 👍

Copy link
Contributor

@drewj-tp drewj-tp left a 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.

  1. When we make a block from blueprints, we have not fully created the assembly, let alone the grid for that assembly.
  2. 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
  3. 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 Outdated Show resolved Hide resolved
armi/reactor/tests/test_blocks.py Outdated Show resolved Hide resolved
armi/reactor/assemblies.py Outdated Show resolved Hide resolved
armi/reactor/blueprints/blockBlueprint.py Show resolved Hide resolved
armi/reactor/blocks.py Show resolved Hide resolved
armi/reactor/blocks.py Outdated Show resolved Hide resolved
armi/reactor/grids/tests/test_grids.py Outdated Show resolved Hide resolved
armi/reactor/grids/tests/test_grids.py Outdated Show resolved Hide resolved
armi/reactor/grids/tests/test_grids.py Outdated Show resolved Hide resolved
armi/reactor/grids/tests/test_grids.py Show resolved Hide resolved
@john-science john-science requested a review from drewj-tp October 11, 2024 22:04
@drewj-tp
Copy link
Contributor

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.

Copy link
Contributor

@drewj-tp drewj-tp left a 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

armi/reactor/blocks.py Outdated Show resolved Hide resolved
armi/reactor/grids/grid.py Outdated Show resolved Hide resolved
armi/reactor/assemblies.py Outdated Show resolved Hide resolved
armi/reactor/blocks.py Outdated Show resolved Hide resolved
armi/reactor/grids/tests/test_grids.py Outdated Show resolved Hide resolved
armi/reactor/cores.py Outdated Show resolved Hide resolved
armi/reactor/tests/test_blocks.py Outdated Show resolved Hide resolved
armi/reactor/tests/test_blocks.py Outdated Show resolved Hide resolved
armi/reactor/tests/test_blocks.py Outdated Show resolved Hide resolved
armi/reactor/tests/test_blocks.py Outdated Show resolved Hide resolved
@john-science john-science marked this pull request as draft October 16, 2024 18:39
@john-science
Copy link
Member Author

@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
Copy link
Member Author

@drewj-tp Progress

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

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 john-science requested a review from drewj-tp October 22, 2024 18:35
@john-science john-science marked this pull request as ready for review October 22, 2024 18:35
Copy link
Contributor

@drewj-tp drewj-tp left a 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.

hex_pins

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()

armi/reactor/blocks.py Outdated Show resolved Hide resolved
armi/reactor/tests/test_blocks.py Show resolved Hide resolved
armi/reactor/tests/test_blocks.py Outdated Show resolved Hide resolved
armi/reactor/blocks.py Outdated Show resolved Hide resolved
@john-science john-science requested a review from drewj-tp October 23, 2024 21:59
Copy link
Contributor

@drewj-tp drewj-tp left a 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 🖖 🚀

armi/_bootstrap.py Show resolved Hide resolved
@john-science john-science merged commit 6b356b3 into main Oct 29, 2024
19 checks passed
@john-science john-science deleted the sub_block_grids branch October 29, 2024 23:51
john-science added a commit that referenced this pull request Oct 30, 2024
drewj-tp added a commit that referenced this pull request Oct 30, 2024
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
Comment on lines +300 to +317
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
Copy link
Member

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:

  1. The method does not raise any errors, so the Raises section of the docstring seems wrong
  2. 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong: Highest Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants