Fix ThetaRZGrid ring/pos bug and update ring/pos docs. #205
Conversation
These are not indexes when in hex: @staticmethod def _indicesAndEdgeFromRingAndPos(ring, position): ring = ring - 1 pos = position - 1 if ring == 0: if pos != 0: raise ValueError(f"Position in center ring must be 1, not {pos}") return 0, 0, 0
armi/reactor/grids.py
Outdated
| @@ -923,9 +923,9 @@ def getLocatorFromRingAndPos(self, ring, pos, k=0): | |||
| Parameters | |||
| ---------- | |||
| ring : int | |||
| Ring index | |||
| Ring index (or number starting at 1 for hex) | |||
There was a problem hiding this comment.
Ring and position are always 1-based
There was a problem hiding this comment.
...and if this is inconsistent in any geometry, then they are wrong
There was a problem hiding this comment.
rzTheta seems to be wrong then:
def getRingPos(self, indices):
return (indices[1] + 1, indices[0] + 1)
@staticmethod
def getIndicesFromRingAndPos(ring, pos):
return (pos, ring)
We are coding in python, so non-zero based indexing is surprising. Also the argument k is clearly zero based indexing, so I thought it would be helpful to point out that this is 1 based.
It is unclear from your feedback what actions you would like me to take. Can you give me some actionable feedback, or let me know if you are okay with these changes?
There was a problem hiding this comment.
Not really sure what you wanted, so I guessed. Let me know if this satisfies your concerns.
There was a problem hiding this comment.
rzTheta is wrong. 100% agree that ring and position being 1-based is surprising, but it is 1-based. The greater sin is to be inconsistent within ring/pos. Could you fix rzTheta?
| @@ -1610,7 +1610,7 @@ def getRingPos(self, indices): | |||
|
|
|||
| @staticmethod | |||
| def getIndicesFromRingAndPos(ring, pos): | |||
| return (pos, ring) | |||
| return (pos - 1, ring - 1) | |||
onufer
changed the title
armi/reactor/grids.py
Outdated
| Not implemented for Cartesian-see getRingPos notes. | ||
| """ | ||
| raise NotImplementedError( | ||
| "Cartesian should not need need ring/pos, use x, y coordinates." |
There was a problem hiding this comment.
"use (i, j) indices instead"
armi/reactor/grids.py
Outdated
| Notes | ||
| ----- | ||
| This is needed to support GUI, but should not often be used. | ||
| x, y (0-based) indices are much more useful. For example: |
armi/reactor/grids.py
Outdated
| This is needed to support GUI, but should not often be used. | ||
| x, y (0-based) indices are much more useful. For example: | ||
|
|
||
| >>> indices = core.spatialGrid[x, y, 0] # 3rd index is 0 for assembly |
armi/reactor/grids.py
Outdated
| >>> indices = core.spatialGrid[x, y, 0] # 3rd index is 0 for assembly | ||
| >>> a = core.childrenByLocator[indices] | ||
|
|
||
| >>> a = core.childrenByLocator[core.spatialGrid[x, y, 0] # one liner |
armi/reactor/grids.py
Outdated
| This is needed to support GUI, but should not often be used. | ||
| i, j (0-based) indices are much more useful. For example: | ||
|
|
||
| >>> indices = core.spatialGrid[i, j, 0] # 3rd index is 0 for assembly |
There was a problem hiding this comment.
locator may be better than indices
There was a problem hiding this comment.
done. will push when build completes
armi/reactor/grids.py
Outdated
| ( 0, 1) ( 1, 0) | ||
| (-1, 1) (0, 0) (1,-1) | ||
| (-1, 0) ( 0,-1) |
Update doc strings for getLocatorFromRingAndPos.
When it is hex geometry 0-based indices are not expected.