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

Transposing pinMgFluxes so that the leading dimension represents pin index #1937

Merged
merged 3 commits into from
Oct 9, 2024
Merged

Transposing pinMgFluxes so that the leading dimension represents pin index #1937

merged 3 commits into from
Oct 9, 2024

Conversation

zachmprince
Copy link
Contributor

@zachmprince zachmprince commented Oct 7, 2024 •
edited
Loading

What is the change?

  • Updating blocks.Block.setPinMgFluxes so the parameters get set assuming the leading dimension is pin index
  • Modifying parameter descriptions to reflect this change.

Why is the change being made?

This is in support of #1860 where it would be easier to rotate pin parameters if they were all consistent on which dimension represents the pin index.

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.

@zachmprince zachmprince requested a review from drewj-tp October 7, 2024 21:20
drewj-tp
drewj-tp requested changes Oct 7, 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.

Can you add a test for this method? We don't have any tests for it here, or else they would also likely need to be updated.

Understanding you were likely not the original author of this method. But it would be great to demonstrate that Block.setPinMgFluxes does what we want

armi/reactor/blocks.py Show resolved Hide resolved
@zachmprince zachmprince force-pushed the assembly_rotation_help branch from c1fa446 to ab569d1 Compare October 8, 2024 01:07
@zachmprince zachmprince marked this pull request as ready for review October 8, 2024 01:07
@zachmprince
Copy link
Contributor Author

Can you add a test for this method? We don't have any tests for it here, or else they would also likely need to be updated.

Understanding you were likely not the original author of this method. But it would be great to demonstrate that Block.setPinMgFluxes does what we want

Good suggestion. I modified the original test to be a bit more robust and actually show that the indexing is correct.

@drewj-tp
Copy link
Contributor

drewj-tp commented Oct 8, 2024

I still think this is a good change. But now that #1860 is closed and we're not rotating the parameter data vectors anymore, this is less vital.

Good, but it does not impact the assembly rotation logic. So long as the structure of the data array is documented

@zachmprince
Copy link
Contributor Author

I would like to keep this change since it makes sense to me, regardless of its impact.

drewj-tp
drewj-tp approved these changes Oct 9, 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'll be sure to come back to this in light of the pinLocation related changes in #1939. But the transposition looks good!

Thanks for the tests too

@john-science john-science merged commit 3d0b871 into terrapower:main Oct 9, 2024
11 checks passed
drewj-tp added a commit that referenced this pull request Oct 11, 2024
@drewj-tp
Merge remote-tracking branch 'origin/main' into drewj/block-input-height
a8b5ef7 
* origin/main:
  Relaxing copyInterfaceInputs to not require a valid Setting (#1934)
  Moving C5G7 into its own test dir (#1941)
  Transposing pinMgFluxes so that the leading dimension represents pin index (#1937)
  Moving Core class to its own module (#1938)
  Removing unusable buildEqRingSchedule (#1928)
  Raising Error when loading inconsistent data from DB (#1940)
  Ensuring we only calculate smear density for pinned blocks (#1933)
  Update rotation requirement impl, tag links: Hex specific (#1936)
  Adding support for ex-core structures (#1891)
  Fixing edge case in assemblyBlueprint._checkParamConsistency (#1929)
drewj-tp added a commit that referenced this pull request Oct 11, 2024
@drewj-tp
Merge remote-tracking branch 'origin/main' into drewj/improve-assem-a…
142d814 
…xial-linkage

* origin/main:
  Relaxing copyInterfaceInputs to not require a valid Setting (#1934)
  Moving C5G7 into its own test dir (#1941)
  Transposing pinMgFluxes so that the leading dimension represents pin index (#1937)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drewj-tp drewj-tp drewj-tp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zachmprince @drewj-tp @john-science