Skip to content
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

Dead parameter rotationDegreesCCW in Block.coords() #1518

Closed
keckler opened this issue Dec 5, 2023 · 1 comment · Fixed by #1651
Closed

Dead parameter rotationDegreesCCW in Block.coords() #1518

keckler opened this issue Dec 5, 2023 · 1 comment · Fixed by #1651
Labels
cleanup Code/comment cleanup: Low Priority good first issue Good for newcomers low priority

Comments

@keckler
Copy link
Member

keckler commented Dec 5, 2023

If one specifies the rotationDegreesCCW parameter to coords(), it will either result in an error or do nothing with it:

armi/armi/reactor/blocks.py

Lines 597 to 607 in 4f55d94

def coords(self, rotationDegreesCCW=0.0):
"""
Returns the coordinates of the block.
.. impl:: Coordinates of a block are queryable.
:id: I_ARMI_BLOCK_POSI1
:implements: R_ARMI_BLOCK_POSI
"""
if rotationDegreesCCW:
raise NotImplementedError("Cannot get coordinates with rotation.")
return self.spatialLocator.getGlobalCoordinates()

armi/armi/reactor/blocks.py

Lines 1620 to 1634 in 4f55d94

def coords(self, rotationDegreesCCW=0.0):
"""
Returns the coordinates of the block.
.. impl:: Coordinates of a block are queryable.
:id: I_ARMI_BLOCK_POSI2
:implements: R_ARMI_BLOCK_POSI
"""
x, y, _z = self.spatialLocator.getGlobalCoordinates()
x += self.p.displacementX * 100.0
y += self.p.displacementY * 100.0
return (
round(x, units.FLOAT_DIMENSION_DECIMALS),
round(y, units.FLOAT_DIMENSION_DECIMALS),
)

I can't find any use of this parameter in any repos that I have access to. I'm not even sure what it would mean for the rotation of a block to impact its location, assuming that orientation is about the centroid of the block.

Extremely low priority, but just pointing it out.

@john-science john-science added help wanted good first issue Good for newcomers cleanup Code/comment cleanup: Low Priority labels Dec 6, 2023
@john-science
Copy link
Member

I agree with Chris' assessment. This parameter looks like it should be removable from both methods.

It's possible that someone is passing unused values into one of those methods, positionally. But (a) unit tests would catch that, and (b) we'd want to know if people were trying to use this defunct parameter anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code/comment cleanup: Low Priority good first issue Good for newcomers low priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants