Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixing HexBlock rotation in plotBlockDiagram #1926

Merged
merged 19 commits into from
Oct 30, 2024
Merged

Fixing HexBlock rotation in plotBlockDiagram #1926

merged 19 commits into from
Oct 30, 2024

Conversation

john-science
Copy link
Member

@john-science john-science commented Oct 4, 2024 •
edited
Loading

What is the change?

Fixed a bug in plotBlockDiagram().

Why is the change being made?

The method plotBlockDiagram() incorrectly plots HexBlocks if they are rotated to the "corners up" rotation.

close #1421

Checklist

  • The release notes have been updated if necessary.
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in pyproject.toml.

@john-science john-science added the bug Something is wrong: Highest Priority label Oct 4, 2024
@john-science john-science requested a review from keckler October 4, 2024 16:28
@keckler
Copy link
Member

keckler commented Oct 4, 2024

First question without even looking at the code -- Can we get a picture of the correct plots?

keckler
keckler reviewed Oct 4, 2024
View reviewed changes
Copy link
Member

@keckler keckler left a 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

armi/reactor/tests/test_blocks.py Outdated Show resolved Hide resolved
armi/utils/plotting.py Outdated Show resolved Hide resolved
armi/utils/plotting.py Outdated Show resolved Hide resolved
@mgjarrett mgjarrett mentioned this pull request Oct 4, 2024
Closed
8 tasks
@john-science john-science requested a review from keckler October 4, 2024 23:00
@keckler
Copy link
Member

keckler commented Oct 5, 2024

@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:

  1. Fixing HexBlock rotation in plotBlockDiagram #1926 (review)
  2. Fixing HexBlock rotation in plotBlockDiagram #1926 (comment)

drewj-tp
drewj-tp reviewed Oct 8, 2024
View reviewed changes
Copy link
Contributor

@drewj-tp drewj-tp left a 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

armi/reactor/blocks.py Outdated Show resolved Hide resolved
armi/reactor/blocks.py Outdated Show resolved Hide resolved
@john-science john-science marked this pull request as draft October 9, 2024 23:13
@john-science
Copy link
Member Author

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.

@john-science
Copy link
Member Author

john-science commented Oct 30, 2024 •
edited
Loading

@keckler and @drewj-tp

Here I plot armiRunSmallest.yaml with the default hex_corners_up:

hex_corners_up

and here I do the same thing, but alter setting the reactor to be "flats up":

hex_flats_up

So, after the recent bug fix, I think we finally have success!

Whew.

@john-science john-science marked this pull request as ready for review October 30, 2024 17:38
@john-science john-science requested a review from drewj-tp October 30, 2024 17:38
keckler
keckler approved these changes Oct 30, 2024
View reviewed changes
Copy link
Member

@keckler keckler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, thank you.

Please update the description in the top comment of the PR. In particular, that HexBlock.cornersUp() method is no longer added.

@john-science john-science merged commit c885a05 into main Oct 30, 2024
19 checks passed
@john-science john-science deleted the fix_plotBlockDiagram branch October 30, 2024 18:53
drewj-tp added a commit that referenced this pull request Oct 30, 2024
@drewj-tp
Merge in pin depletion related changes
5b8e76a 
Merge remote-tracking branch 'zprince/component_flux_up' into drewj/bu-rotate-with-pin-dep

* zprince/component_flux_up:
  start rm'img things
  Fixing HexBlock rotation in plotBlockDiagram (#1926)
  Removing defunct import (#1986)
  Fixing wetted perimeter for hex inner ducts (#1985)
  Fixing sub-block grids (#1947)
  make history tracker respect control assemblies
  comment out print statements
  add defaults to pin level params
  add c.p.puFrac calc, move b.getPuMoles up the composite tree, add a pu frac comp params
  add c.p.molesHmBOL and populate it
  Adding component method to retrieve pin fluxes
  Revert "Moving pin flux parameters to component level"
  add pinPercentBu component param
  Moving pin flux parameters to component level
@keckler keckler restored the fix_plotBlockDiagram branch December 13, 2024 22:11
@keckler keckler deleted the fix_plotBlockDiagram branch December 13, 2024 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keckler keckler keckler approved these changes

@drewj-tp drewj-tp Awaiting requested review from drewj-tp

Assignees
No one assigned
Labels
bug Something is wrong: Highest Priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

plotBlockDiagram() doesn't respect block orientation
3 participants
@john-science @keckler @drewj-tp