Add New Component Axial Linking Approach #1376
Conversation
Now need to do something with them....
- axial exp changer starting to account for block grids. - works when lattices have different numbers of pins. - need to change for case when pins are the same number in the grid (this should generate linking problems)
- still requires all pinned blocks to have the same number of pin groupings - but can now use the spatial locators of components to resolve multiple axial linkage
- uses a smarter grid-based approach. Score! - needs cleanup + unit testing
- also make it a class method to include self.a (instead of parent.parent....)
- during axial expansion choose the component which will be used for expansion. - all tests pass. Need to add aditional testing for coverage, improve docstrings, and see if logic optimization is available
- add testing to cover complexities
albeanth
changed the title
There was a problem hiding this comment.
This looks very good. I like that your new test provides a good blueprint for somebody to implement this in their model.
I think the only thing that requires real attention is that I think the logic no longer tests if the components are solids. It'd be great if you could check on that.
|
|
||
| Notes | ||
| ----- | ||
| 3 cases are considered, see test_axialExpansionChanger.py::TestRetriveAxialLinkage for details. |
There was a problem hiding this comment.
I think it could be more helpful to the user if this were reversed, i.e. put the description in this docstring and then reference it from the test docstring. People rarely look at docstrings for tests, so having the bulk of the description here instead seems to make more sense.
| maxCompZtop = 0.0 | ||
| for otherC in linkedComponents: | ||
| if otherC.ztop > maxCompZtop: | ||
| linked = otherC | ||
| maxCompZtop = otherC.ztop |
There was a problem hiding this comment.
| maxCompZtop = 0.0 | |
| for otherC in linkedComponents: | |
| if otherC.ztop > maxCompZtop: | |
| linked = otherC | |
| maxCompZtop = otherC.ztop | |
| linked = max(linkedComponents, key=lambda c: c.ztop) |
I think that accomplishes the same thing you're doing here.
| self.linkedComponents[c] = lstLinkedC | ||
|
|
||
| if lstLinkedC[0] is None: | ||
| if AssemblyAxialLinkage._determineLinked(c, otherC): |
There was a problem hiding this comment.
Genuinely curious -- is there a strong benefit to using AssemblyAxialLinkage._determineLinked() as compared to self._determineLinked() here?
| component to compare and see if is linked to componentA | ||
| Notes | ||
| ----- | ||
| If componentA and componentB are both solids and the same type, geometric overlap can be checked via |
There was a problem hiding this comment.
I think your change lost the check on whether the components are solids or not. Am I wrong about that?
| # check for common indices between components. If either component has indices within its counterpart, | ||
| # then they are candidates to be linked and overlap should be checked. |
There was a problem hiding this comment.
Just thinking out loud here... It seems like this logic will not allow for partial-length fuel rods, if I'm reading it correctly. That's not an issue for me right now, but just trying to consider common cases (partial-length rods are common in BWRs).
| @@ -19,3 +19,26 @@ core: | |||
| IC IC MC OC | |||
| AF IC IC PC SH | |||
| symmetry: third periodic | |||
|
|
|||
| fourPin: | |||
| # geom: hex | |||
|
|
|
I do not know why this branch is now seeing some test failures in the |
|
Okay, after some offline discussion with @albeanth , the changes that I mentioned in the comment above have been rolled back. Instead of trying to handle them, we are explicitly going to block users from having blocks in their assemblies that don't have any solid components in them. In such scenarios, it is unclear how the axial expansion should be performed and so we just disallow it. The exceptions to this are:
I added the appropriate logic and an associated test. |
| if len(solidCompsInBlock) == 0: | ||
| raise InputError( | ||
| f"Assembly {self.linked.a} is constructed improperly for use with the axial expansion changer.\n" | ||
| "Consider using the assemFlagsToSkipAxialExpansion case setting." |
There was a problem hiding this comment.
Let's give a little more detail so users don't have to dig into the source code. E.g. "...is constructed improperly for use with the axial expansion changer; i.e., assemblies cannot have intermediate blocks void of a solid component. Consider using....
There was a problem hiding this comment.
Well ideally there would be some documentation that outlines how to use the axial expansion capability ;-)
There was a problem hiding this comment.
Hah! Yeah yeah.... very true....
…CompAxialLinkMthd
|
@keckler a subtle hint hint you want this sooner than later, eh? 😄 |
|
I can't run my model without this branch, so at the very least I needed to get the merge conflicts resolved. But yes, sooner is better! |
|
I will carve some out time this week to restart this effort. I know John would like to see this PR gone too! |
|
Note to self: check in on BOL parameters on fuel. They're currently at the block-level and this change may break the validity of those parameters. They may need to get shifted to the components. |
What is the change?
Closes #769. A request was made to improve the axial linking logic to take advantage of the block grids system in ARMI. This PR aims to accomplish that. While previous behavior is maintained, there are several changes:
i. A block grid was added to armi/tests/detailedAxialExpansion/refSmallCoreGrid.yaml
ii. Additional complexity was added to the simple assembly defined in test_blockBlueprints.py.
Why is the change being made?
To support a user request for downstream analyses.
Checklist
doc/release/0.X.rst) are up-to-date with any important changes.docfolder. Another item for Formal Documentation of Axial Expansion in ARMI #935....setup.py.