HexBlock.rotate updates child spatial locators
What is the change?
Calls to HexBlock.rotate update the spatial locator on all child components. This is necessary because you'd expect rotation to move things in space.
Tests are added showing
- pin coordinates, as reported by
Block.getPinCoordinatesreflect the rotation - location objects, reported by
Component.spatialLocatorreflect the rotation
Other changes were made in support of this functionality.
What about pinLocation
It still exists but is not modified at all during calls to HexBlock.rotate. Routines that were using pinLocation for a pre/post mapping should be okay, as evidenced by the lack of changes to any pinLocation related tests (e.g., setPinPowers). The mapping is transparent because it remains a list of one-indexed "pin locations" such that data[pinLocations[i] - 1] is still data[i] since pinLocations[i] == i + 1.
HexGrid.rotateIndex
To facilitate the rotation, I added HexGrid.rotateIndex to take in an index location, rotate it some number of 60-degree rotations, and return the new location. I drew a lot of information and support from https://www.redblobgames.com/grids/hexagons/ (hereafter RBG) that I'll try to synthesize here.
ARMI's hex grid indexing is very similar to the "axial" or "trapezoidal" coordinate system described in RBG. Some key differences
- We use
i,jto define location whereas RBG usesq,r. - In the RBG examples, their global Y direction is opposite ours, i.e., our grid is flipped along the x-axis. This means a clockwise rotation in our coordinate system is a counterclockwise rotation in theirs, and vice-versa.
Rotation of items on a hex grid is almost trivial using what RBG refers to as a cubic coordinate system, or where a point on the 2-D grid can be defined by three coordinates: q, r, s. The conversion between axial and cube just means computing s = -q - r.
60-degree rotation in this system is created by shuffling and negating the points. Following their examples, starting at location (q, r, s), increasing CW rotations would place a cell at
-
(-r, -s, -q) -
(s, q, r) -
(-q, -r, -s) - and so on.
After three rotations, we have gone 180 degrees so it tracks that our locations is essentially the same coordinates but opposite sign.
This is tested by rotating a point some amount, and comparing the XY coordinates in the cartesian plane according to a rotation matrix
[[ cos(theta), -sin(theta)]
[ sin(theta), cos(theta)]]
Block.getPinLocations for obtaining index locations
Might want to change the name to not use "location" because that feels confusing w/ getPinCoordinates. Maybe getPinIndices? getPinIndexLocations?
Anyway. Some talk with internal stakeholders implied, to me, that mapping between ARMI's pin ordering and ordering in other applications would be more suited with integer indices rather than Cartesian XY coordinates. So Block.getPinLocations will produce all the IndexLocation objects for pins.
Block.getPinCoordinates returns numpy array
It felt odd to get a list of arrays? This should be back compatible for things expecting list of arrays, as the getPinCoordinates test was not updated to use (N, 3) array features until later
- Returns array in 1c603a2d
- Test updated in 0a599b27
I guess not fully back compatible if you're doing pop/append/index operations on the coordinates... I can revert this change if we want to save it for later. Just made sense as a code-cleanup operation.
Why is the change being made?
Closes #1939
Personal Todo
- [x] Docs on rotation, and how to find where data like
linPowByPinexists in space - ~~[ ] Remove usage of
pinLocationin pin data setters likesetPinPowers(could be follow on since it still works, it's just unnecessary)~~ Not necessary at this time
Checklist
- [x] This PR has only one purpose or idea.
- [x] Tests have been added/updated to verify any new/changed code.
- [x] The code style follows good practices.
- [x] The commit message(s) follow good practices.
- [x] The release notes have been updated if necessary.
- [x] The documentation is still up-to-date in the
docfolder. - [x] The dependencies are still up-to-date in
pyproject.toml.
There is at least one project that is slightly broken b/c Block.getPinCoordinates returns an array and not a list. That's a trivial fix for me. Otherwise, this is ready for review (but not merge just yet)
@drewj-tp Do you expect your solution here to need changing after the sub-block grid fix? https://github.com/terrapower/armi/pull/1947
@drewj-tp Do you expect your solution here to need changing after the sub-block grid fix? #1947
I'll check but I don't think so. The rotation logic shoves most of the hard work onto the grid, so we should be okay?
Had some downstream differences that I couldn't quite square until I removed
- Changes to
HexBlock.autoCreateSpatialGrids- Also being modified in #1947
- Implementation of
MultiIndexLocation.getLocalCoordinates- Maybe the "bug" I thought I found was a product of a bad test configuration? For whatever reason, I'm not hitting a potentially related problem that caused me to look at this method