-
Notifications
You must be signed in to change notification settings - Fork 87
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
Refactor Axial Expansion #1435
Refactor Axial Expansion #1435
Conversation
…asses into their own files, and move a few methods onto __init__.py.
…ineLinked a static method
- also improve the frequency in which the runLogs get hit
|
Yes. I will add one once we get further into the review.
The API actually should not have changed at all actually. At least not to that I can remember off the top of my head. I am going to run this on the slough of internal tests though to confirm. |
The API for unit testing did change. And the refactor changed some imports downstream. So there is an another (minor) downstream PR. |
@john-science @keckler giving a bump on this. just started doing QA work for the axial expansion changer and it would be really nice to push this through. the improved unit testing and accompanying refactor will help keep things clean imo. happy to chat offline too. |
Put something on my calendar for this, @albeanth |
I'll put something on your calendar for next week. Some of the downstream testing is freaking out. I have no idea why and don't have enough time to figure it out this week unfortunately... |
and isinstance(componentA, type(componentB)) | ||
and (componentA.getDimension("mult") == componentB.getDimension("mult")) | ||
): | ||
if isinstance(componentA, UnshapedComponent): |
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 about NullComponent?
armi/reactor/converters/axialExpansion/axialExpansionChanger.py
Outdated
Show resolved
Hide resolved
armi/reactor/converters/axialExpansion/axialExpansionChanger.py
Outdated
Show resolved
Hide resolved
armi/reactor/converters/axialExpansion/axialExpansionChanger.py
Outdated
Show resolved
Hide resolved
armi/reactor/converters/axialExpansion/axialExpansionChanger.py
Outdated
Show resolved
Hide resolved
""" | ||
blkLst = self.linked.a.getBlocks() | ||
if not blkLst[-1].hasFlags(Flags.DUMMY): | ||
runLog.warning( |
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.
Do we have a unit test that covers the warnings and errors in this method?
(The primary cause of code coverage being low in ARMI right now is people adding a near infinite number of warnings, but not testing them. In the future, I am wondering if all need warnings and exceptions should be covered by tests. Though I haven't made that rule yet, obviously.)
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.
These should have testing, yes. I made a concerted effort a year or two ago to make sure that each of these had coverage and that axial expansion had 100% testing coverage. It should be the same...!
runLog.debug( | ||
"Printing component expansion information (growth percentage and 'target component')" | ||
f"for each block in assembly {self.linked.a}." | ||
) | ||
for ib, b in enumerate(self.linked.a): | ||
runLog.debug(msg=f" Block {b}") |
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.
Out of curiosity, do you think anyone but you will ever want to see these debug
lines?
My concern is that they look like something you added because they were useful to you developing axial expansion?
|
||
class AxialExpansionChanger: | ||
""" | ||
Axially expand or contract assemblies or an entire core. |
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.
It appears to me (tell me if I'm wrong!) that axial expansion only works on a pin-type (assembly-type) reactor.
Assuming that's true, I think this might be something more like:
Axially expand or contract assemblies or an entire core of assemblies.
But more so, I wonder if there is another high-level place or two we should call out that this feature only works on assemblies. Thoughts?
armi/reactor/converters/axialExpansion/axialExpansionChanger.py
Outdated
Show resolved
Hide resolved
armi/reactor/converters/axialExpansion/tests/buildAxialExpAssembly.py
Outdated
Show resolved
Hide resolved
return b | ||
|
||
|
||
def buildDummySodium(hotTemp: float, height: float): |
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.
For some reason, this name seems too short to me. Maybe add "Block"?
buildDummySodiumBlock
Thoughts?
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 not sure if it's documented somewhere, but maybe explaining the significance of a DUMMY
name would be valuable as a test with the flag system. Essentially you might want a test that demonstrates this method is returning the expected thing if not already checked by itself in a test just in case this flag gets broken later by accident.
armi/reactor/converters/axialExpansion/tests/test_assemblyAxialLinkage.py
Outdated
Show resolved
Hide resolved
Co-authored-by: John Stilley <[email protected]>
Co-authored-by: John Stilley <[email protected]>
@albeanth - I'm curious if you've tried different ways to use the axial expansion code in an unintended way, like calling methods out of order or with different, unexpected block types or geometries. Essentially, I'm wondering if there are sufficient checks on the Assembly design (e.g., hex, Cartesian, RZ, etc.) to guard against excepted behavior. If so, would you be able to point out some of those unit tests or if not, could you add some? For example, what about an assembly with custom ISO topics as the materials? What about homogeneous blocks with no components? |
I am closing this PR for a new one: #1861 Tony is aware, and anything lost in this PR we can steal for the new one (or another one). We are closing the PR, not deleting our history. |
What is the change?
This PR does a fairly large refactor of the axial expansion functionality in ARMI. A new directory is structure is created to enable better traceability with testing and general maintainability.
Original:
and unit tests:
New:
and unit tests
Notes:
This does include the changes from PR #1427.
Why is the change being made?
axialExpansionChanger.py
file was responsible for too many things. It was ready to be split up to improve maintainability.test_axialExpansionChanger.py
was likewise too big and doing too much. It has now been split up and is easier to maintain.Checklist
doc/release/0.X.rst
) are up-to-date with any important changes.doc
folder.pyproject.toml
.