-
Notifications
You must be signed in to change notification settings - Fork 90
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.
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
HexBlock.rotate updates child spatial locators #1943
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add requirement test and implementation crumbs
Most methods seem to have their own copying in place so let them do that per test. But create the entire block once, then let deepcopy create the unique copies.
Otherwise an error is raised trying to get local coordinates on a pin-like component when the grid is auto-created
Maybe it's left out to avoid pre-populating sites that aren't needed? But when we're creating a spatial grid on the assumption we have a full grid of things, why not pass in the number of rings we expect automatically?
There were some errors raised in the invocation of _evaluateMesh only expecting a single entry, errors like ``` result[self._stepDims] = stepCoords ValueError: shape mismatch: value array of shape (3,3) could not be broadcast to indexing result of shape (3,) ``` Now, `MultiIndex.getLocationCoordinates` will produce the local coords for everything contained in the index. And a test is added. so that's good!
Rely on a cubic coordinate system for rotation. Yes, that's confusing because we're hexagons. It's really a 3-d indexing such that ``(q, r, s) = (i, j, -(i + j))``. Rotation is much simpler here because we can shift these values left once and negate them for each counter clockwise rotation. So, starting at ``(q, r, s)``, the new "cubic" coordinates after increasing rotations would be 1. ``(-r, -s, -q)`` 2. ``(s, q, r)`` 3. ``(-q, -r, -s)`` and so on. This draws on the glorious https://www.redblobgames.com/grids/hexagons/ page for guidance. There are minor deviations that are worth calling out here 1. ARMI orientation is referred to as axial coorinates (aka trapezoidal or oblique or skewed) 2. ARMI orientation is flipped across the x-axis in their examples. So our +y global cartesian vector is their -y global cartesian vector. The real difference is our CCW rotation is their CW rotation because of this flip.
Compare the local coordinates with a translation matrix to show we get similar results.
Some applications may find integer indices easier to work with than xyz coordinates. getPinCoords is an array because a list of arrays is inefficient.
Need to specially consider CoordinateLocation objects but should be nearly there.
It doesn't matter how many times we rotate the center location, we should get the center
Unsure if we should then override the grid and set the grid of the new object to be `self`. For now, let it ride
This reverts commit 5d35026.
Had some downstream differences that I couldn't quite square until I removed
|
Co-authored-by: John Stilley <1831479+john-science@users.noreply.github.com>
john-science
approved these changes
Oct 23, 2024
zachmprince
approved these changes
Oct 23, 2024
john-science
approved these changes
Oct 25, 2024
drewj-tp
added a commit
that referenced
this pull request
Oct 29, 2024
* main: Fixing HexBlock docstrings (#1981) Removing a duplicate hex rotation impl tag (#1979) Avoiding closing plots that are meant to be interactive (#1978) Ensuring HexBlock.rotate updates child spatial locators (#1943) Add new memory runLog info to memoryProfiler.py (#1970) Fixing various doc build issues (#1974) Hiding sphinx-needs warnings during doc build (#1973) Fixing warnings in CLI startup (#1972)
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Block.getPinCoordinates
reflect the rotationComponent.spatialLocator
reflect the rotationOther 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 usingpinLocation
for a pre/post mapping should be okay, as evidenced by the lack of changes to anypinLocation
related tests (e.g.,setPinPowers
). The mapping is transparent because it remains a list of one-indexed "pin locations" such thatdata[pinLocations[i] - 1]
is stilldata[i]
sincepinLocations[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
i,j
to define location whereas RBG usesq,r
.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 computings = -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)
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
Block.getPinLocations
for obtaining index locationsMight want to change the name to not use "location" because that feels confusing w/
getPinCoordinates
. MaybegetPinIndices
?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 theIndexLocation
objects for pins.Block.getPinCoordinates
returns numpy arrayIt 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 laterI 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
linPowByPin
exists in space[ ] Remove usage ofNot necessary at this timepinLocation
in pin data setters likesetPinPowers
(could be follow on since it still works, it's just unnecessary)Checklist
doc
folder.pyproject.toml
.