-
Notifications
You must be signed in to change notification settings - Fork 89
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
Use clearer input syntax for hexagonal lattice pitch #1654
base: main
Are you sure you want to change the base?
Conversation
I'm just thinking aloud here, but what something "flat-to-flat" be better for hex grids? Isn't the goal to call out that we are going to be explicitly picking one over the other? |
Calculating the pitch with a square root creates a machine precision error.
I'm not sure I understand what you are proposing. Do you mean specifying the pitch/flat-to-flat distance as |
The bug fix was pulled out of this PR and into #1649 The remaining part of this PR is an optional/nice-to-have feature. It's just allowing users to define the hex pitch in blueprints in a slightly different way than before. No rush to push this through now, really. |
The unit tests broke after my PR merged. Sorry! |
No worries! I just have to pull out what was moved over to #1649 and I think it will work. |
This pull request has been automatically marked as stale because it has not had any activity in the last 100 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. |
@mgjarrett Forgive me, I have totally lost where this PR was at. When you think it is ready for review, take it out of Draft mode. |
I think the PR is ready. For context, I was trying to address the issue that Drew and Mitch were discussing 3 years ago over on this issue #252: |
I had unintentionally added some files that were sitting around in my git directory to this PR. These files have been removed, so this PR is now ready. |
@mgjarrett Does this PR require changes downstream? (Is it backwards compatible, or do we need to fix dozens of downstream input files after merging this PR?) |
@@ -360,7 +361,8 @@ def tearDown(self): | |||
|
|||
def test_simpleRead(self): | |||
gridDesign = self.grids["control"] | |||
_ = gridDesign.construct() | |||
grid = gridDesign.construct() | |||
self.assertAlmostEqual(grid.pitch, 1.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 was thinking it would be nice to show that this works for corners up and flats up.
I was think you could copy/paste this test, and just search/replace "geom: hex_corners_up"
with "geom: hex
.
Thoughts?
What is the change?
Added an option for a user to specify a hexagonal pitch directly:
Why is the change being made?
Previously, a user had to specify the triangular pitch using the
x
attribute oflatticePitch
, which is confusing because the pitch is not necessarily equal to thex
distance between two neighbors in a hex grid.https://github.com/terrapower/armi/blob/main/armi/reactor/grids/hexagonal.py#L152-L161
Fix 1284(fix for this issue pulled into a different PR, Fix some bugs in "corners up" Hex Grids #1649)Checklist
doc
folder.pyproject.toml
.