armi icon indicating copy to clipboard operation
armi copied to clipboard

HexBlock.rotate updates child spatial locators

Open drewj-tp opened this issue 1 year ago • 4 comments

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

  1. pin coordinates, as reported by Block.getPinCoordinates reflect the rotation
  2. location objects, reported by Component.spatialLocator reflect 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

  1. We use i,j to define location whereas RBG uses q,r.
  2. 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

  1. (-r, -s, -q)
  2. (s, q, r)
  3. (-q, -r, -s)
  4. 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 linPowByPin exists in space
  • ~~[ ] Remove usage of pinLocation in pin data setters like setPinPowers (could be follow on since it still works, it's just unnecessary)~~ Not necessary at this time

Checklist

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

drewj-tp avatar Oct 10 '24 16:10 drewj-tp

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 avatar Oct 10 '24 23:10 drewj-tp

@drewj-tp Do you expect your solution here to need changing after the sub-block grid fix? https://github.com/terrapower/armi/pull/1947

john-science avatar Oct 11 '24 16:10 john-science

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

drewj-tp avatar Oct 11 '24 19:10 drewj-tp

Had some downstream differences that I couldn't quite square until I removed

  1. Changes to HexBlock.autoCreateSpatialGrids
    • Also being modified in #1947
  2. 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

drewj-tp avatar Oct 21 '24 18:10 drewj-tp