Rotate more parameters in HexBlock.rotate #1877

drewj-tp · 2024-09-13T18:00:48Z

What is the change?

Calls to HexBlock.rotate will additionally rotate parameters on the block with the ParamLocation.CHILDREN tag.

Why is the change being made?

Rotating a block previously did not update all of the relevant parameters.

Closes #1860

Checklist

This PR has only one purpose or idea.
Tests have been added/updated to verify any new/changed code.

The code style follows good practices.
The commit message(s) follow good practices.

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.

Can and will be used to signify that a parmeter should be rotated during calls to Block.rotate and Assembly.rotate.

Should make documentation a lot more powerful, linking the information directly to the attribute.

First step in showing intent for better parameter rotation #1860

…ters

drewj-tp · 2024-09-13T18:01:27Z

cc @albeanth @onufer
who else should peek at this?

drewj-tp · 2024-09-13T19:32:49Z

Tests are failing because we have a test that does HexAssembly.rotate and then does extensive testing on parameters on the hex block. These are redundant if we also have extensive testing on HexBlock.rotate.

I'm of the opinion those should be moved to the block tests. Then we just need to make sure that HexAssembly.rotate calls HexBlock.rotate

albeanth · 2024-09-13T21:12:38Z

cc @albeanth @onufer who else should peek at this?

Maybe @alexhjames since he wrote some of the rotation stuff, I believe

albeanth · 2024-09-13T21:19:20Z

Tests are failing because we have a test that does HexAssembly.rotate and then does extensive testing on parameters on the hex block. These are redundant if we also have extensive testing on HexBlock.rotate.

I'm of the opinion those should be moved to the block tests. Then we just need to make sure that HexAssembly.rotate calls HexBlock.rotate

I haven't look at the tests in question, but if they are repetitive and can be moved, then I think it's probably fair to do.

onufer · 2024-09-16T15:02:49Z

i agree with this. it makes no sense to rotate rates (flux/fast flux/dpa rate). When you rotate an assembly the solution changes so the rates need to be recalculated (and we do that). Integrated params (fluence, fast fluence, dpa) should be rotated and we intend to. Thanks!

alexhjames · 2024-09-17T16:19:55Z

i agree with this. it makes no sense to rotate rates (flux/fast flux/dpa rate). When you rotate an assembly the solution changes so the rates need to be recalculated (and we do that). Integrated params (fluence, fast fluence, dpa) should be rotated and we intend to. Thanks!

Not sure I agree with this. The rotate method gets called both during shuffling, in which case your comment makes sense, but also during growToFullCore where it doesn't. The assemblies that are out of the first third need ALL spatially varying parameters rotated as well.

maybe we should have two methods, one for growing to full core and one for shuffling? Like a
rotate vs a reflect?

alexhjames · 2024-09-17T16:23:42Z

armi/reactor/blocks.py

+ -----
+ Parameters marked with the ``rotatable``
+ :class:`~armi.reactor.parameters.parameterDefinitions.Category` will be rotated.
+ These parameters must adhere to one of the following rules to be rotated:


Missing vector valued quantities like displacementX and displacementY where two block parameters form a vector. Alternatively, you could restructure displacementX and displacmentY to be a single parameter that follows these rules.

Yes that was not called out in the docs. I've revamped the PR and kept the existing rotation routines (displacement, orientation) so those should be unimpacted

drewj-tp · 2024-09-17T18:02:11Z

Summarizing a separate chat.

During rotation from shuffling and moving assemblies, some parameters will be wrong because they don't match the state of the reactor. And that's acceptable for now.

I'll make a separate ticket to capture that

drewj-tp · 2024-09-17T18:02:50Z

maybe we should have two methods, one for growing to full core and one for shuffling? Like a rotate vs a reflect?

If we were only rotating certain parameters during shuffling-based HexBlock.rotate and doing a lot more in growToFullCore, this could be a good strategy. My intention now is to rotate everything that looks rotatable in HexBlock.rotate so the reflect method may not be needed now.

This reverts commit 4162cd7.

This reverts commit c1e638a.

This reverts commit 8fcd775.

…ate" This reverts commit cac6b6f.

…e parameters" This reverts commit 2cdcd77.

Imagine storing neutron, gamma, and total multi-group flux per cell. That is not something we currently do, but it's not outside the realm of something someone might want to do. We do store multigroup flux per hex cell, but that would still be a vector of data per cell.

This gets harder to make consise if we check 200+ entries. So instead, the test checks 1. Are the first two rings exactly correct? Using existing helper functionality. 2. If we rotated, are all entries except the center different? 3. If we did not, or rotated a full 360 degrees, did we get the original array? 4. If we rotate again to reach the original state (e.g., 2 and then 4 rotations), do we get the original data?

Felt redundant. Rather infer the number of sites in a full hexagonal lattice and compare against that.

…parameters

drewj-tp · 2024-09-20T18:38:17Z

okay @HunterPSmith @john-science @alexhjames @onufer this feels much better now.

Existing rotation is not touched. Duct params, orientation, and displacement should be rotated as they were before.

Any block parameter with the ParamLocation.CHILDREN will be rotated, assuming it has enough entries to fill a full hexagonal lattice. So if you have three hex rings of things, you should provide an array with 19 items.

A couple cases where this could be a problem:

You have empty sites so there are no pins at certain locations.
You have intermixed pin types (e.g., fuel and coolant) on different lattice sites

A concrete example of where this could be a problem would be the MHTGR 350 benchmark (#224). The assemblies have fuel compacts and coolant channels on the same hexagonal lattice. Users would have to make sure the things that are populating Block.p["linPowByPin"] and Block.p["percentBuByPin"] provide empty data the coolant sites.

If we want to remove that assumption, we need to find a way to pass through the pin positions. Maybe with the spatial grid locations? But that's not clear to me right now.

doc/release/0.4.rst

armi/reactor/blocks.py

john-science · 2024-09-20T20:45:03Z

armi/reactor/blocks.py

@@ -2167,6 +2175,25 @@ def _rotatePins(self, rotNum, justCompute=False):

 return rotateIndexLookup

+ def _rotatePinParameters(self, rotNum: int):


Could use a one-line comment. WE had to talk about this a lot, so there must be something interesting to say about it.

Up to you though; not a deal breaker.

Good with me

john-science · 2024-09-20T20:45:29Z

Love the tests, man!

armi/reactor/tests/test_blocks.py

Co-authored-by: John Stilley <[email protected]>

doc/release/0.4.rst

armi/reactor/blocks.py

drewj-tp · 2024-09-20T22:25:41Z

armi/reactor/blocks.py

@@ -2167,6 +2175,25 @@ def _rotatePins(self, rotNum, justCompute=False):

 return rotateIndexLookup

+ def _rotatePinParameters(self, rotNum: int):


Good with me

drewj-tp · 2024-09-20T22:30:07Z

armi/reactor/parameters/parameterDefinitions.py

- """
- A "namespace" for storing parameter categories.
-
- Notes
- -----
- * `cumulative` parameters are accumulated over many time steps
- * `pinQuantities` parameters are defined on the pin level within a block
- * `multiGroupQuantities` parameters have group dependence (often a 1D numpy array)
- * `fluxQuantities` parameters are related to neutron or gamma flux
- * `neutronics` parameters are calculated in a neutronics global flux solve
- * `gamma` parameters are calculated in a fixed-source gamma solve
- * `detailedAxialExpansion` parameters are marked as such so that they are mapped from the
- uniform mesh back to the non-uniform mesh
- * `reactivity coefficients` parameters are related to reactivity coefficient or kinetics
- parameters for kinetics solutions
- * `thermal hydraulics` parameters come from a thermal hydraulics physics plugin (e.g., flow
- rates, temperatures, etc.)
- """
+ """A "namespace" for storing parameter categories."""

 depletion = "depletion"
+ """Parameters used in or calculated by a depletion plugin."""
 cumulative = "cumulative"
+ """Parameters are accumulated over many time steps"""
 cumulativeOverCycle = "cumulative over cycle"
+ """Parameters that are reset at beginning of cycle and accumulated over each cycle."""
 assignInBlueprints = "assign in blueprints"
+ """Parameters that should be assigned in blueprints (e.g., control rod elevation)"""
 retainOnReplacement = "retain on replacement"
 pinQuantities = "pinQuantities"
+ """Parameters are defined on the pin level within a block"""
 fluxQuantities = "fluxQuantities"
+ """Parameters are related to neutron or gamma flux"""


(This came up in a chat so wanted to put my motivation here)

The change here is to make these docstrings attached to the class constants. They were already documented in the Category docstring (some of them at least) but I found two flaws

They were documented in the Notes section but would make more sense to be properly documented as class attributes

Attaching the docstring to the object itself means tools like python, ipython, vscode, etc. can provide context where you are about the thing you're looking at. You don't need to separately pull up the ARMI docs.

I'm not going to die on this hill if people have stronger opinions. This is just my rationale for this change.

drewj-tp added 10 commits September 13, 2024 08:34

Fix docstring for parameterDefinitions.Category

71ca23d

Move Block rotate tests to separate TestCase

2221d28

Add test for HexBlock rotate boundary parameters

acf46f6

Provide Parameter.hasCategory

4162cd7

Add parameter Category.rotatable

c1e638a

Can and will be used to signify that a parmeter should be rotated during calls to Block.rotate and Assembly.rotate.

Move Category docstrings to attributes

4215ce7

Should make documentation a lot more powerful, linking the information directly to the attribute.

Add some type hints to parameter methods

dea16bc

Mark pointsEdgeDpa and pointsCornerDpa as rotatable parameters

8fcd775

Only rotate pointsCornerDpa and pointsEdgeDpa in HexBlock.rotate

cac6b6f

First step in showing intent for better parameter rotation #1860

Add release note about HexBlock.rotate only rotating rotatable parame…

2cdcd77

…ters

drewj-tp requested review from john-science and HunterPSmith September 13, 2024 18:00

drewj-tp self-assigned this Sep 13, 2024

drewj-tp mentioned this pull request Sep 16, 2024

Provide a way to query parameters with a category #1884

Open

alexhjames reviewed Sep 17, 2024

View reviewed changes

drewj-tp mentioned this pull request Sep 17, 2024

Provide Parameter.hasCategory #1886

Closed

7 tasks

drewj-tp mentioned this pull request Sep 17, 2024

Some parameters after shuffing and rotating don't match reactor's state #1890

Open

drewj-tp added 5 commits September 18, 2024 15:00

Revert "Provide Parameter.hasCategory"

7ff0914

This reverts commit 4162cd7.

Revert "Add parameter Category.rotatable"

ba32f6f

This reverts commit c1e638a.

Revert "Mark pointsEdgeDpa and pointsCornerDpa as rotatable parameters"

0474b59

This reverts commit 8fcd775.

Revert "Only rotate pointsCornerDpa and pointsEdgeDpa in HexBlock.rot…

e8ded77

…ate" This reverts commit cac6b6f.

Revert "Add release note about HexBlock.rotate only rotating rotatabl…

4549227

…e parameters" This reverts commit 2cdcd77.

drewj-tp added 18 commits September 20, 2024 08:35

Improve docstrings for TestHexCellRotate

5f63878

Test rotation of hex data for three rings

fdd8c43

Change a helper test method name to be more explicit

3494e4d

Fix rotateHexCellData to account for number of cells per edge

bd5e662

Fix rotatedHexCellData to cound entries along first axis

a580b3a

Add test that hex cell data in lists can also be rotated

bcbac9c

Docs for rotateHexCellData

0c944b4

rotateHexCellData no longer takes cells entry

1c174e7

Felt redundant. Rather infer the number of sites in a full hexagonal lattice and compare against that.

Flush out more Block.rotate checks for pin data

de16d2a

Add release note for HexBlock.rotate rotating ParamLocation.CHILDREN …

28d16d8

…parameters

Add docstring for Category.thermalHydraulics

14bb890

Add docstring for ParamLocation.CHILDREN

63a4095

Allow rotateHexCellData to do nothing on empty data

4741505

Add more useful exception if block parameter fails to be rotated

ca410f0

Add test for invalid rotatable data types

586b850

Merge branch 'main' into drewj/rotate-block-params/1860

52f7d50

drewj-tp requested review from john-science and alexhjames September 20, 2024 18:30

drewj-tp marked this pull request as ready for review September 20, 2024 18:30

john-science reviewed Sep 20, 2024

View reviewed changes

doc/release/0.4.rst Outdated Show resolved Hide resolved

john-science reviewed Sep 20, 2024

View reviewed changes

armi/reactor/blocks.py Show resolved Hide resolved

john-science reviewed Sep 20, 2024

View reviewed changes

armi/reactor/tests/test_blocks.py Outdated Show resolved Hide resolved

Apply suggestions from code review

f893079

Co-authored-by: John Stilley <[email protected]>

drewj-tp commented Sep 20, 2024

View reviewed changes

drewj-tp mentioned this pull request Sep 20, 2024

Allow a way to efficiently iterate over some parameters #1898

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rotate more parameters in HexBlock.rotate #1877

Rotate more parameters in HexBlock.rotate #1877

drewj-tp commented Sep 13, 2024 •

edited

Loading

drewj-tp commented Sep 13, 2024

drewj-tp commented Sep 13, 2024

albeanth commented Sep 13, 2024

albeanth commented Sep 13, 2024

onufer commented Sep 16, 2024

alexhjames commented Sep 17, 2024

alexhjames Sep 17, 2024

drewj-tp Sep 20, 2024

drewj-tp commented Sep 17, 2024

drewj-tp commented Sep 17, 2024

drewj-tp commented Sep 20, 2024

john-science Sep 20, 2024

drewj-tp Sep 20, 2024

john-science commented Sep 20, 2024

drewj-tp Sep 20, 2024

drewj-tp Sep 20, 2024

		@@ -2167,6 +2175,25 @@ def _rotatePins(self, rotNum, justCompute=False):

		return rotateIndexLookup

		def _rotatePinParameters(self, rotNum: int):

Rotate more parameters in HexBlock.rotate #1877

Are you sure you want to change the base?

Rotate more parameters in HexBlock.rotate #1877

Conversation

drewj-tp commented Sep 13, 2024 • edited Loading

What is the change?

Why is the change being made?

Checklist

drewj-tp commented Sep 13, 2024

drewj-tp commented Sep 13, 2024

albeanth commented Sep 13, 2024

albeanth commented Sep 13, 2024

onufer commented Sep 16, 2024

alexhjames commented Sep 17, 2024

alexhjames Sep 17, 2024

Choose a reason for hiding this comment

drewj-tp Sep 20, 2024

Choose a reason for hiding this comment

drewj-tp commented Sep 17, 2024

drewj-tp commented Sep 17, 2024

drewj-tp commented Sep 20, 2024

john-science Sep 20, 2024

Choose a reason for hiding this comment

drewj-tp Sep 20, 2024

Choose a reason for hiding this comment

john-science commented Sep 20, 2024

drewj-tp Sep 20, 2024

Choose a reason for hiding this comment

drewj-tp Sep 20, 2024

Choose a reason for hiding this comment

drewj-tp commented Sep 13, 2024 •

edited

Loading