-
Notifications
You must be signed in to change notification settings - Fork 94
Make axial linking aware of block grids for axial expansion #2145
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
base: mixed-pin-assemblies
Are you sure you want to change the base?
Make axial linking aware of block grids for axial expansion #2145
Conversation
…t_liquids need to get moved to a new test that tests areAxiallyLinked and not _checkOverlap directly
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.
Relevant changes to this file:
- simplified some of the dimensions and consistency with component temps.
- replaced the
lead test fuel
assembly with a mixed pin assembly,multi pin fuel
. - removed the
feed fuel
andlead test fuel b
assemblies (I can't recall which, but some weren't used in the lattice map).
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.
Updated the lattice map to be consistent with the new assembly defs and added the block-grid.
for newB in newAssem: | ||
sourceB = sourceAssem.getBlockAtElevation(newB.p.z) |
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.
This change was needed because with the new blueprints def, the newAssem
didn't have the same number of blocks as the sourceAssem
.
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.
I would LOVE to clean this up and split it into multiple files.... But I didn't want to make the changes so drastic is was harder to review than necessary
EDIT: I did split out axial linking testing into its own file. Was easier to manage.
@@ -117,8 +152,7 @@ class AxialLink(typing.Generic[Comp]): | |||
* :attr:`AxialAssemblyLinkage.linkedComponents` | |||
""" | |||
|
|||
lower: typing.Optional[Comp] = dataclasses.field(default=None) | |||
upper: typing.Optional[Comp] = dataclasses.field(default=None) |
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.
So turns out, we never use upper
and having it complicated the new linking. We can bring it back if you feel strongly.
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.
If we weren't using it, probably seems we don't need it. I guess you would want to use it to understand what this component is pushing into and/or sending information into. But we still understand the overall linking based on every component knowing what's below it. Kind of list a singly linked list vs. doubly linked list
elif isinstance(componentA.spatialLocator, MultiIndexLocation) and isinstance( | ||
componentB.spatialLocator, MultiIndexLocation | ||
): | ||
## Case 2 |
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.
I bet the stuff under this conditional could be made more elegant/pythonic... open to suggestions!
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.
Could we use a Set[tuple[int, int]]
where each tuple is the ij
location for that index? Then we can compare the sets directly. Maybe
fromA = set((index.i, index.j) for index in componentA.spatialLocator.indices)
fromB = set((index.i, index.j) for index in componentB.spatialLocator.indices)
if fromA == fromB:
linked = _checkOverlap(componentA, componentB)
else:
linked = False
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.
e40089a
much simpler actually matches what is intended -- all indices must mach. I should have looked closer here! I must have been trying to get fancy when I originally wrote this
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.
sooooo I was actually mistaken. we do not want to always require a perfect match. I will make updates and ping you.
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.
I'm very curious to find out why
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.
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.
Conversely, we could not allow this and require exact matches. This might better represent reality as the pin stacks for fuel 1 and fuel 2 are likely different designs, so it makes less sense to say they share the same shield pin component.
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.
Hmmm okay, I would want to bring in people who'd be making the inputs. But I feel like a partial match is going to be more trouble than it's worth.
Especially as we look at updating things between linked components. That feels more prone to stumbling than if we enforce 1:1
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.
I chatted with @mgjarrett and we will enforce 1:1 and split the shield component into two different components.
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.
So requirements: I don't think this work has any effect on existing requirements. Nor do I necessarily think we need to add any new ones. armi/doc/qa_docs/srsd/reactors_reqs.rst Lines 182 to 217 in c23e5e6
We could possibly add unit tests to the acceptance testing, but I don't think it's necessary. Am open to discussion though. |
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.
Partial review. The implementation side of things give a lot of confidence. There's one big thing I think to resolve and test: exact match in indices or are we okay with a subset.
I am partially through reviewing the changes to testing and I'm liking what I'm seeing
armi/reactor/assemblies.py
Outdated
plenumVolume += math.pi * (cladId / 2.0) ** 2.0 * length * 1e-6 | ||
return plenumVolume |
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.
Micro-opt: increment the squared radii for each clad component and then scale by pi * length * 1e-6
at the end
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.
met ya halfway. I thought keeping the volume calc together (pi r^2 h) was nice for readability but did pull the unit conversion out to the end
386b8df
armi/reactor/blocks.py
Outdated
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.
All the changes here seem to be related to #2142
Does the base branch need to be updated to track main?
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.
should be good now
armi/reactor/components/component.py
Outdated
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.
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.
should be good now
"""Case 4; component type mismatch.""" | ||
compDims = ("test", "FakeMat", 25.0, 25.0) # name, material, Tinput, Thot | ||
comp1 = Circle(*compDims, od=1.0, id=0.0) | ||
comp2 = UnshapedComponent(*compDims, area=1.0) |
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.
Since we do special logic for unshaped components, I would recommend making the second component a hexagon or something. Just so we don't have any overlap (pun intended) with case 1: unshaped components
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.
0080e09 good call
comp2.p.mult = 2 | ||
self.assertFalse(AssemblyAxialLinkage.areAxiallyLinked(comp1, comp2)) | ||
|
||
def test_multiIndexLocation(self): |
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.
Request for a test that shows either
- Locations can be a partial match, or
- Locations cannot be a partial match
I'm inclined to think we want the latter (exact match in locations) but other comments/parts of the code have me unsure
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.
you are correct, we want the latter. They have to match exactly.
if assertionBool: | ||
self.assertTrue(_checkOverlap(typeA, typeB), msg=msg) | ||
self.assertTrue(_checkOverlap(typeB, typeA), msg=msg) | ||
else: | ||
self.assertFalse(_checkOverlap(typeA, typeB), msg=msg) | ||
self.assertFalse(_checkOverlap(typeB, typeA), msg=msg) |
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.
What if we made the assertion function an argument to the test? e.g., self.runTest(..., self.assertTrue)
? Non-blocking discussion not really a request but it would collapse six lines to two without sacrificing readability in the later tests
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.
1241895 agreed totally
|
||
def runTest( | ||
self, | ||
componentsToTest: dict, |
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.
(sarcastic) come on, we can do better w/ the types!
I think dict[Type[Component], dict[str, float]]
would be what we want
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.
💯 577a88a
Co-authored-by: Drew Johnson <[email protected]>
c6c382c
to
e84c018
Compare
64d91b5
to
e694041
Compare
45b9092
to
ea6348b
Compare
What is the change? Why is it being made?
This is the replacement for #1376.
What: This change enables the axial linking class supporting axial expansion to be aware of the grid blocks. This enables users to have assembly definitions that contain multiple pin-components (e.g., mixing fuel and reflector pins).
Why: Increases the capabilities of ARMI by allowing more assembly types to be modeled.
SCR Information
Change Type: features
One-Sentence Description: Enable multi-pin component support for axial expansion.
One-line Impact on Requirements: N/A
Checklist
doc
folder.pyproject.toml
.