-
Notifications
You must be signed in to change notification settings - Fork 90
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
Fixing HexBlock rotation in plotBlockDiagram #1926
base: main
Are you sure you want to change the base?
Conversation
First question without even looking at the code -- Can we get a picture of the correct plots? |
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 think also this chunk might need to be made aware of the orientation:
https://github.com/terrapower/armi/pull/1926/files#diff-0c52725b9d66a2bcefb306c31e4a9479fe8c206453c22b3e4cd268a1efef3c16R1181-R1185
@john-science Thank you for fixing those few comments. Please also note that there are a couple more things that would be good before approving this PR: |
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 currently poking around grid like things and some things here overlapped
def cornersUp(self): | ||
"""Determine if the hex shape of is corners up or flats up, in relation to the Y axis.""" | ||
if self.spatialGrid is None: | ||
return 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.
I guess this is false-like? But returning None
from a bool-like method feels weird.
Would it make more sense to return False or error here?
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.
A Python None
is a null value. It is just as valid to use None
for a null value when a return type is an int
, numpy.ndarray
or bool
.
Seems standard enough to return an empty value from a method.
What would you rather do? I don't want to return an error, because this does not seem like a scenario in which I want to kill a simulation.
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
@drewj-tp Well, in defense of your idea, I am still getting this: It appears the bug for the hexagonal shaped composites was quite easy to fix. But the coordinates for the pins are just wrong from the start. (There's nothing I can do in the plotting routine to fix that.) |
This PR is temporarily on hold for another PR: #1947 I believe my solution in this was over-engineered to work AROUND the problem present in the other PR. Once that other PR merges, I should be able to simplify this PR. |
What is the change?
plotBlockDiagram()
.HexBlock.cornersUp()
, as I imagine it will be a helpful utility to have.self.HexBlock
toself.hexBlock
, to match our variable naming rules.Why is the change being made?
The method
plotBlockDiagram()
incorrectly plotsHexBlock
s if they are rotated to the "corners up" rotation.close #1421
Checklist
doc
folder.pyproject.toml
.