Extensibility improvements to axial expansion classes #1920
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Use early `return` statements to dedent the function. Renamed to `areAxiallyLinked` to be a bit more declarative, even though this function lives in the axial expansion routine. Add some type hints
The values in the linkage dictionaries on `AssemblyAxialLinkage` were lists of two items where the position meant something: `0` for linked item below the current thing, and `1` for linked item above the current thing. Some functionality was based on `links[1] is not None` which is not super readable unless you're familiar with what's going on. This introduces a small data class with two attributes: `lower` and `upper` such that `links.lower is links[0]` and `links.upper is links[1]`. This improves readability and extensibility by making a more explicit and declarative thing. The position-based getters and setters are retained and tested. The class is "templated" with a `typing.Generic[typing.TypeVar]` that could be either `Block` or `Component`. This is because the the two linked dictionaries on the `AssemblyAxialLinkage` class only differ based on the types of their values. We still need to know lower/upper as determined by the values, but for the block link dict we want blocks and then components from the component link dict.
Also rename componentLst to components in setExpansionFactors in an effort to remove type names from variable names
Single method that iterate over all blocks in an assembly and then all components in each block is now broken across a per-block method and a per-component method. This distinction will also allow subclasses to provide specific expansion methods on a more granular level.
The getSolidComponents produces a list which may not be needed in all situations. Lots of usage of getSolidComponents was for iteration, e.g., `for c in getSolidComponents(b)` where producing an iterable is sufficient.
Move docstring to class definition and outside `__init__` for consistency.
Useful for testing what you get is what you expect. Tests added to this effect. Renamed an internal variable from `componentWFlag` to `candidates` because the name implied `componentWFlag` was a single component when it was a (potentially empty) list of components. Finally, bring out the setting of the target component to a separate method. This is useful for subclasses that may need custom logic to determine their target component, but want to perform the same actions on the setter.
Useful for subclasses to override. Calls back to existing function for small code change.
drewj-tp
commented
Sep 30, 2024
This implementation an O(N^2) iteration where each block also iterates over all blocks in the assembly. The use of itertools.chain and itertools.islice provide a more efficient and streamlined iteration look. The call signature is for a `Sequence[Block]` which could just as easily be `Assembly`. We can't use just `Iterable[Block]` because we need to perform multiple iterations on the object at once. And things like a generator may get consumed part way through the iteration. And for testing, passing a list of blocks gets the same behavior as passing one assembly. Tests for zero blocks, one block, and multiple blocks added.
john-science
reviewed
Oct 3, 2024
armi/reactor/converters/axialExpansionChanger/assemblyAxialLinkage.py
Outdated
Show resolved
Hide resolved
john-science
reviewed
Oct 3, 2024
armi/reactor/converters/axialExpansionChanger/assemblyAxialLinkage.py
Outdated
Show resolved
Hide resolved
john-science
reviewed
Oct 3, 2024
john-science
approved these changes
Oct 3, 2024
…kage.py Co-authored-by: John Stilley <[email protected]>
albeanth
reviewed
Oct 4, 2024
armi/reactor/converters/axialExpansionChanger/assemblyAxialLinkage.py
Outdated
Show resolved
Hide resolved
albeanth
reviewed
Oct 4, 2024
albeanth
reviewed
Oct 4, 2024
albeanth
reviewed
Oct 7, 2024
| @@ -217,15 +246,21 @@ def _setTargetComponents(self, setFuel): | |||
| elif b.hasFlags(Flags.PLENUM) or b.hasFlags(Flags.ACLP): | |||
There was a problem hiding this comment.
I can't comment above, but the content in the if b.p.axialExpTargetComponent block above should be replaced by a call to _setExpansionTarget.
Co-authored-by: Tony Alberti <[email protected]>
Co-authored-by: Tony Alberti <[email protected]>
…e' into drewj/improve-assem-axial-linkage * origin/drewj/improve-assem-axial-linkage: Apply suggestions from code review
Only really provided for back compatability with the existing list API. But this is far enough down the axial expansion API, which already has dubious widespread usage, that we're okay removing this unceremoniously.
albeanth
approved these changes
Oct 8, 2024
|
@john-science this PR is apart of a larger downstream internal change, so please hold off on merging this. thank you! |
…xial-linkage * origin/main: Removing defunct comment from getPinCoordinates (#1931)
Co-authored-by: Tony Alberti <[email protected]>
…-axial-linkage' into drewj/improve-assem-axial-linkage * refs/remotes/origin/drewj/improve-assem-axial-linkage: Black Update armi/reactor/converters/tests/test_axialExpansionChanger.py
…xial-linkage * origin/main: New plugin hook for before reactor construction (#1945) Removing DoseResultsMapper (#1952) Adding logic so HexAssemblies are hexagonal (#1935) Adding Block.getInputHeight (#1927) Removing Assembly.doubleResolution (#1951) Adding DeprecationWarning for HistoryTrackerInterface (#1950) Removing tabulate as a dependency (#1948)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is the change?
A handful of improvements to allow subclasses of axial expansion things more control. A general theme was to take large methods and break them up so subclasses can override at their discretion.
Some documentation has been added to axial expansion related classes. It's by no means complete, nor to the level of #935. But as I fumbled through the code and got helped from @albeanth, I added what I could to the docstrings.
Why is the change being made?
I am working on features internally that need more control over performing axial expansion that require these changes.
Closes #1453
Closes #1918
Checklist
docfolder.pyproject.toml.