Repository: terrapower/armi

Issue:
Add some logic so HexAssemblies are hexagonal
Right now, the classes `HexAssembly` and `CartesianAssembly` are just empty slates:

https://github.com/terrapower/armi/blob/8b9873cfe9df8dac6a26b28a7520c45b65fa3a50/armi/reactor/assemblies.py#L1229-L1237

https://github.com/terrapower/armi/blob/8b9873cfe9df8dac6a26b28a7520c45b65fa3a50/armi/reactor/assemblies.py#L1240-L1241

But they give people the assumption they can trust these assemblies have a shape, or the blocks inside them do.  But obviously no.

I recommend that we override the superclass method `Assembly.add()` here to check the shape of the Blocks added to assemblies of these types. That will give users some trust, anyway, that instances of these classes are what you'd think at first glance.

Discussion:
john-science: @jeffbaylor @alexhjames Just FYI.
jakehader: Hi @john-science - how we feel about removing the Hex, Cartesian, and THRZ classes? We used to have a bunch of logic in these subclasses but they were removed in favor of minimizing. I don't think an assembly "shape" truly matters. I think that THRZ is the odd one here and it might be better to clean that one up to remove shape dependencies altogether.

Really, an assembly should just be a stacked list of 1D blocks, where most of the magic happens.
drewj-tp: > Hi @john-science - how we feel about removing the Hex, Cartesian, and THRZ classes? We used to have a bunch of logic in these subclasses but they were removed in favor of minimizing. I don't think an assembly "shape" truly matters. I think that THRZ is the odd one here and it might be better to clean that one up to remove shape dependencies altogether.

@jakehader I think this is good fodder for https://github.com/terrapower/armi/discussions/1896. I also think it makes sense to have shapiness on assemblies too. Even if to just check that you don't add an incompatible block-type to an assembly. Finding a cartesian block amongst a bunch of hex blocks feels like something that could be desirable to avoid

> Really, an assembly should just be a stacked list of 1D blocks, where most of the magic happens.

Agree to an extent. If we re-named the _base_ assembly class to be something like `BlockStack`, that would allow reactors that don't have assemblies (PBRs, MSRs) to still use _some_ of the API even though they don't have assemblies in the SFR/LWR/HTGR sense.

See maybe #1030?
john-science: Okay, perhaps I will re-work this PR so that we ONLY have `Assembly`, which has an attribute `a.blockType`. And if `.blockType` is set (optional), then that `Assembly` will only accept `Block`s of that type.

My _guess_ is that is a small change that can happen only in ARMI and not involve any downstream changes. And then we can leave the bigger, high-level changes to the "abstraction" Discussion.

If it turns out such a change will require many downstream changes, I will revisit the idea.
john-science: @jakehader @drewj-tp Okay, per our discussion here and over on [here](https://github.com/terrapower/armi/discussions/1896), I modified my solution here. The generic `Assembly` class now just has a class attribute `.blockType` that can be `HexBlock` for `HexAssembly`.

This is closer to your end game.

However, the full end game of, say, removing `HexAssembly` , for instance, is way beyond the scope of this ticket. We are going to have a higher level discussion about ARMI's abstract classes, and hopefully that will go in the next release. (But not this one.)

Patch:
diff --git a/armi/reactor/assemblies.py b/armi/reactor/assemblies.py
index c26d74d6..28574e17 100644
--- a/armi/reactor/assemblies.py
+++ b/armi/reactor/assemblies.py
@@ -15,25 +15,26 @@
 """
 Assemblies are collections of Blocks.
 
-Generally, blocks are stacked from bottom to top.
+Generally, Blocks are stacked from bottom to top.
 """
 import copy
 import math
 import pickle
 from random import randint
+from typing import ClassVar, Optional, Type
 
 import numpy as np
 from scipy import interpolate
 
 from armi import runLog
-from armi.reactor.spentFuelPool import SpentFuelPool
+from armi.materials.material import Fluid
 from armi.reactor import assemblyParameters
 from armi.reactor import blocks
 from armi.reactor import composites
 from armi.reactor import grids
 from armi.reactor.flags import Flags
-from armi.materials.material import Fluid
 from armi.reactor.parameters import ParamLocation
+from armi.reactor.spentFuelPool import SpentFuelPool
 
 
 class Assembly(composites.Composite):
@@ -42,14 +43,15 @@ class Assembly(composites.Composite):
     Append blocks to add them up. Index blocks with 0 being the bottom.
     """
 
+    _BLOCK_TYPE: ClassVar[Optional[Type[blocks.Block]]] = None
     pDefs = assemblyParameters.getAssemblyParameterDefinitions()
 
-    LOAD_QUEUE = "LoadQueue"
-    SPENT_FUEL_POOL = "SFP"
     # For assemblies coming in from the database, waiting to be loaded to their old
     # position. This is a necessary distinction, since we need to make sure that a bunch
     # of fuel management stuff doesn't treat its re-placement into the core as a new move
     DATABASE = "database"
+    LOAD_QUEUE = "LoadQueue"
+    SPENT_FUEL_POOL = "SFP"
     NOT_IN_CORE = [LOAD_QUEUE, SPENT_FUEL_POOL]
 
     def __init__(self, typ, assemNum=None):
@@ -58,7 +60,6 @@ class Assembly(composites.Composite):
         ----------
         typ : str
             Name of assembly design (e.g. the name from the blueprints input file).
-
         assemNum : int, optional
             The unique ID number of this assembly. If None is provided, we generate a
             random int. This makes it clear that it is a placeholder. When an assembly with
@@ -87,14 +88,12 @@ class Assembly(composites.Composite):
 
         Notes
         -----
-        As with other ArmiObjects, Assemblies are sorted based on location. Assemblies
-        are more permissive in the grid consistency checks to accomodate situations
-        where assemblies might be children of the same Core, but not in the same grid as
-        each other (as can be the case in the spent fuel pool). In these situations,
-        the operator returns ``False``.  This behavior may lead to some strange sorting
-        behavior when two or more Assemblies are being compared that do not live in the
-        same grid. It may be beneficial in the future to maintain the more strict behavior
-        of ArmiObject's ``__lt__`` implementation once the SFP situation is cleared up.
+        As with other ArmiObjects, Assemblies are sorted based on location. Assemblies are more
+        permissive in the grid consistency checks to accomodate situations where assemblies might be
+        children of the same Core, but not in the same grid as each other (like in the spent fuel
+        pool). In these situations, the operator returns ``False``.  This behavior may lead to some
+        strange sorting behavior when two or more Assemblies are being compared that do not live in
+        the same grid.
 
         See Also
         --------
@@ -160,6 +159,17 @@ class Assembly(composites.Composite):
         self.p.assemNum = randint(-9000000000000, -1)
         self.renumber(self.p.assemNum)
 
+    def _checkPotentialChild(self, obj: blocks.Block, action: str = "add"):
+        """An internal helper method to ensure the Block type is valid for this Assembly."""
+        if self._BLOCK_TYPE is None or isinstance(obj, self._BLOCK_TYPE):
+            # this is the right Block, pass on
+            return
+
+        # if we got here, this Block is not the right type for this Assembly
+        msg = f"Cannot {action} {obj} to this Assembly, it is not a {self._BLOCK_TYPE}."
+        runLog.error(msg)
+        raise TypeError(msg)
+
     def add(self, obj: blocks.Block):
         """
         Add an object to this assembly.
@@ -180,6 +190,7 @@ class Assembly(composites.Composite):
             are updated. The axial mesh and other Block geometry parameters are
             updated in ``calculateZCoords``.
         """
+        self._checkPotentialChild(obj, "add")
         composites.Composite.add(self, obj)
         obj.spatialLocator = self.spatialGrid[0, 0, len(self) - 1]
 
@@ -187,6 +198,12 @@ class Assembly(composites.Composite):
         self.reestablishBlockOrder()
         self.calculateZCoords()
 
+    def insert(self, index, obj):
+        """Insert an object at a given index position with the assembly."""
+        self._checkPotentialChild(obj, "insert")
+        composites.Composite.insert(self, index, obj)
+        obj.spatialLocator = self.spatialGrid[0, 0, index]
+
     def moveTo(self, locator):
         """Move an assembly somewhere else."""
         composites.Composite.moveTo(self, locator)
@@ -197,11 +214,6 @@ class Assembly(composites.Composite):
         # symmetry may have changed (either moving on or off of symmetry line)
         self.clearCache()
 
-    def insert(self, index, obj):
-        """Insert an object at a given index position with the assembly."""
-        composites.Composite.insert(self, index, obj)
-        obj.spatialLocator = self.spatialGrid[0, 0, index]
-
     def getNum(self):
         """Return unique integer for this assembly."""
         return int(self.p.assemNum)
@@ -315,38 +327,6 @@ class Assembly(composites.Composite):
 
         return sum(plenumTemps) / len(plenumTemps)
 
-    def doubleResolution(self):
-        """
-        Turns each block into two half-size blocks.
-
-        Notes
-        -----
-        Used for mesh sensitivity studies.
-
-        Warning
-        -------
-        This is likely destined for a geometry converter rather than this instance method.
-        """
-        newBlockStack = []
-        topIndex = -1
-        for b in self:
-            b0 = b
-            b1 = copy.deepcopy(b)
-            for bx in [b0, b1]:
-                newHeight = bx.getHeight() / 2.0
-                bx.p.height = newHeight
-                bx.p.heightBOL = newHeight
-                topIndex += 1
-                bx.p.topIndex = topIndex
-                newBlockStack.append(bx)
-                bx.clearCache()
-
-        self.removeAll()
-        self.spatialGrid = grids.AxialGrid.fromNCells(len(newBlockStack))
-        for b in newBlockStack:
-            self.add(b)
-        self.reestablishBlockOrder()
-
     def adjustResolution(self, refA):
         """Split the blocks in this assembly to have the same mesh structure as refA."""
         newBlockStack = []
@@ -1252,7 +1232,9 @@ class Assembly(composites.Composite):
 
 
 class HexAssembly(Assembly):
-    """Placeholder, so users can explicitly define a hex-based Assembly."""
+    """An assembly that is hexagonal in cross-section."""
+
+    _BLOCK_TYPE = blocks.HexBlock
 
     def rotate(self, rad: float):
         """Rotate an assembly and its children.
@@ -1279,27 +1261,31 @@ class HexAssembly(Assembly):
         """
         if math.isclose(rad % (math.pi / 3), 0, abs_tol=1e-12):
             return super().rotate(rad)
-        raise ValueError(
-            f"Rotation must be in 60 degree increments, got {math.degrees(rad)} "
-            f"degrees ({rad} radians)"
+
+        msg = (
+            f"Rotation must be in 60 degree increments, got {math.degrees(rad)} degrees "
+            f"({rad} radians)."
         )
+        runLog.error(msg)
+        raise ValueError(msg)
 
 
 class CartesianAssembly(Assembly):
-    pass
+    """An assembly that is rectangular in cross-section."""
+
+    _BLOCK_TYPE = blocks.CartesianBlock
 
 
 class RZAssembly(Assembly):
     """
-    RZAssembly are assemblies in RZ geometry; they need to be different objects than
-    HexAssembly because they use different locations and need to have Radial Meshes in
-    their setting.
+    RZAssembly are assemblies in RZ geometry; they need to be different objects than HexAssembly
+    because they use different locations and need to have Radial Meshes in their setting.
 
     Notes
     -----
-    ThRZAssemblies should be a subclass of Assemblies (similar to Hex-Z) because
-    they should have a common place to put information about subdividing the global mesh
-    for transport - this is similar to how blocks have 'AxialMesh' in their blocks.
+    ThRZAssemblies should be a subclass of Assemblies because they should have a common place to put
+    information about subdividing the global mesh for transport. This is similar to how blocks have
+    'AxialMesh' in their blocks.
     """
 
     def __init__(self, name, assemNum=None):
@@ -1307,43 +1293,19 @@ class RZAssembly(Assembly):
         self.p.RadMesh = 1
 
     def radialOuter(self):
-        """
-        Returns the outer radial boundary of this assembly.
-
-        See Also
-        --------
-        armi.reactor.blocks.ThRZBlock.radialOuter
-        """
+        """Returns the outer radial boundary of this assembly."""
         return self[0].radialOuter()
 
     def radialInner(self):
-        """
-        Returns the inner radial boundary of this assembly.
-
-        See Also
-        --------
-        armi.reactor.blocks.ThRZBlock.radialInner
-        """
+        """Returns the inner radial boundary of this assembly."""
         return self[0].radialInner()
 
     def thetaOuter(self):
-        """
-        Returns the outer azimuthal boundary of this assembly.
-
-        See Also
-        --------
-        armi.reactor.blocks.ThRZBlock.thetaOuter
-        """
+        """Returns the outer azimuthal boundary of this assembly."""
         return self[0].thetaOuter()
 
     def thetaInner(self):
-        """
-        Returns the outer azimuthal boundary of this assembly.
-
-        See Also
-        --------
-        armi.reactor.blocks.ThRZBlock.thetaInner
-        """
+        """Returns the outer azimuthal boundary of this assembly."""
         return self[0].thetaInner()
 
 
@@ -1352,11 +1314,6 @@ class ThRZAssembly(RZAssembly):
     ThRZAssembly are assemblies in ThetaRZ geometry, they need to be different objects
     than HexAssembly because they use different locations and need to have Radial Meshes
     in their setting.
-
-    Notes
-    -----
-    This is a subclass of RZAssemblies, which is itself a subclass of the generic
-    Assembly class.
     """
 
     def __init__(self, assemType, assemNum=None):
diff --git a/armi/reactor/blocks.py b/armi/reactor/blocks.py
index c2e7466b..9210ca5b 100644
--- a/armi/reactor/blocks.py
+++ b/armi/reactor/blocks.py
@@ -1727,6 +1727,34 @@ class Block(composites.Composite):
         u8 = self.getMass("U238")
         return u5 / (u8 + u5)
 
+    def getInputHeight(self) -> float:
+        """Determine the input height from blueprints.
+
+        Returns
+        -------
+        float
+            Height for this block pulled from the blueprints.
+
+        Raises
+        ------
+        AttributeError
+            If no ancestor of this block contains the input blueprints. Blueprints are
+            usually stored on the reactor object, which is typically an ancestor of
+            the block (block -> assembly -> core -> reactor). However, this may be the case
+            when creating blocks from scratch in testing where the entire composite
+            tree may not exist.
+        """
+        ancestorWithBp = self.getAncestor(
+            lambda o: getattr(o, "blueprints", None) is not None
+        )
+        if ancestorWithBp is not None:
+            bp = ancestorWithBp.blueprints
+            assemDesign = bp.assemDesigns[self.parent.getType()]
+            heights = assemDesign.height
+            myIndex = self.parent.index(self)
+            return heights[myIndex]
+        raise AttributeError(f"No ancestor of {self} has blueprints")
+
 
 class HexBlock(Block):
     """
diff --git a/doc/release/0.4.rst b/doc/release/0.4.rst
index d14ce049..3a4726a9 100644
--- a/doc/release/0.4.rst
+++ b/doc/release/0.4.rst
@@ -19,6 +19,7 @@ New Features
 #. Provide ``Parameter.hasCategory`` for quickly checking if a parameter is defined with a given category. (`PR#1899 <https://github.com/terrapower/armi/pull/1899>`_)
 #. Provide ``ParameterCollection.where`` for efficient iteration over parameters who's definition matches a given condition. (`PR#1899 <https://github.com/terrapower/armi/pull/1899>`_)
 #. Plugins can provide the ``getAxialExpansionChanger`` hook to customize axial expansion. (`PR#1870 <https://github.com/terrapower/armi/pull/1870`_)
+#. Provide ``Block.getInputHeight`` for determining the height of a block from blueprints. (`PR#1927 <https://github.com/terrapower/armi/pull/1927`_)
 #. TBD
 
 API Changes
@@ -32,6 +33,7 @@ API Changes
 #. ``copyInterfaceInputs`` no longer requires a valid setting object. (`PR#1934 <https://github.com/terrapower/armi/pull/1934>`_)
 #. Removing ``buildEqRingSchedule``. (`PR#1928 <https://github.com/terrapower/armi/pull/1928>`_)
 #. Allowing for unknown Flags when opening a DB. (`PR#1844 <https://github.com/terrapower/armi/pull/1835>`_)
+#. Removing ``Assembly.doubleResolution()``. (`PR#1951 <https://github.com/terrapower/armi/pull/1951>`_)
 #. Removing ``assemblyLists.py`` and the ``AssemblyList`` class. (`PR#1891 <https://github.com/terrapower/armi/pull/1891>`_)
 #. Removing ``Assembly.rotatePins`` and ``Block.rotatePins``. Prefer ``Assembly.rotate`` and ``Block.rotate``. (`PR#1846 <https://github.com/terrapower/armi/1846`_)
 #. Transposing ``pinMgFluxes`` parameters so that leading dimension is pin index (`PR#1937 <https://github.com/terrapower/armi/pull/1937>`)


Test patch:
diff --git a/armi/reactor/tests/test_assemblies.py b/armi/reactor/tests/test_assemblies.py
index ebc4cac8..56c21d91 100644
--- a/armi/reactor/tests/test_assemblies.py
+++ b/armi/reactor/tests/test_assemblies.py
@@ -333,6 +333,9 @@ class Assembly_TestCase(unittest.TestCase):
             self.assertEqual(b.parent, a)
             self.assertEqual(len(a), n + 1)
 
+        with self.assertRaises(TypeError):
+            a.add(blocks.CartesianBlock("Test Cart Block"))
+
     def test_moveTo(self):
         ref = self.r.core.spatialGrid.getLocatorFromRingAndPos(3, 10)
         i, j = grids.HexGrid.getIndicesFromRingAndPos(3, 10)
@@ -392,20 +395,6 @@ class Assembly_TestCase(unittest.TestCase):
         places = 6
         self.assertAlmostEqual(cur, ref, places=places)
 
-    def test_doubleResolution(self):
-        b = self.assembly[0]
-        initialHeight = b.p.heightBOL
-        self.assembly.doubleResolution()
-        cur = len(self.assembly.getBlocks())
-        ref = 2 * len(self.blockList)
-        self.assertEqual(cur, ref)
-
-        cur = self.assembly.getBlocks()[0].getHeight()
-        ref = self.height / 2.0
-        places = 6
-        self.assertNotEqual(initialHeight, b.p.heightBOL)
-        self.assertAlmostEqual(cur, ref, places=places)
-
     def test_adjustResolution(self):
         # Make a second assembly with 4 times the resolution
         assemNum2 = self.assemNum * 4
diff --git a/armi/reactor/tests/test_blocks.py b/armi/reactor/tests/test_blocks.py
index d9e1b51a..d53fc397 100644
--- a/armi/reactor/tests/test_blocks.py
+++ b/armi/reactor/tests/test_blocks.py
@@ -34,6 +34,7 @@ from armi.physics.neutronics.settings import (
 from armi.reactor import blocks, blueprints, components, geometry, grids
 from armi.reactor.components import basicShapes, complexShapes
 from armi.reactor.flags import Flags
+from armi.reactor.tests.test_reactors import loadTestReactor
 from armi.reactor.tests.test_assemblies import makeTestAssembly
 from armi.tests import ISOAA_PATH, TEST_ROOT
 from armi.utils import hexagon, units
@@ -1797,6 +1798,39 @@ class Block_TestCase(unittest.TestCase):
         )
 
 
+class BlockInputHeightsTests(unittest.TestCase):
+    def test_foundReactor(self):
+        """Test the input height is pullable from blueprints."""
+        r = loadTestReactor()[1]
+        msg = "Input height from blueprints differs. Did a blueprint get updated and not this test?"
+
+        # Grab a block from an assembly, so long as we have the height
+        assem = r.core.getFirstAssembly(Flags.IGNITER | Flags.FUEL)
+        lowerB = assem[0]
+        self.assertEqual(
+            lowerB.getInputHeight(),
+            25,
+            msg=msg,
+        )
+        # Grab another block just for good measure
+        midBlock = assem[2]
+        self.assertEqual(
+            midBlock.getInputHeight(),
+            25,
+            msg=msg,
+        )
+        # Top block has a different height. Make sure we don't just
+        # return 25 all the time
+        topBlock = assem[4]
+        self.assertEqual(topBlock.getInputHeight(), 75, msg=msg)
+
+    def test_noBlueprints(self):
+        """Verify an error is raised if there are no blueprints."""
+        b = buildSimpleFuelBlock()
+        with self.assertRaisesRegex(AttributeError, "No ancestor.*blueprints"):
+            b.getInputHeight()
+
+
 class BlockEnergyDepositionConstants(unittest.TestCase):
     """Tests the energy deposition methods.
 
diff --git a/armi/reactor/tests/test_reactors.py b/armi/reactor/tests/test_reactors.py
index a64d1dc7..6187155e 100644
--- a/armi/reactor/tests/test_reactors.py
+++ b/armi/reactor/tests/test_reactors.py
@@ -1347,6 +1347,15 @@ class CartesianReactorTests(ReactorTests):
         self.o = buildOperatorOfEmptyCartesianBlocks()
         self.r = self.o.r
 
+    def test_add(self):
+        a = self.r.core.getFirstAssembly()
+        numA = len(a)
+        a.add(blocks.CartesianBlock("test cart block"))
+        self.assertEqual(len(a), numA + 1)
+
+        with self.assertRaises(TypeError):
+            a.add(blocks.HexBlock("test hex block"))
+
     def test_getAssemblyPitch(self):
         # Cartesian pitch should have 2 dims since it could be a rectangle that is not square.
         assert_equal(self.r.core.getAssemblyPitch(), [10.0, 16.0])