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

DM-32055: Write util for calculating angle between wcses #611

Merged
merged 2 commits into from Oct 14, 2021

Conversation

mfisherlevine
Copy link
Contributor

No description provided.

@mfisherlevine mfisherlevine force-pushed the tickets/DM-32055 branch 2 times, most recently from e84e80c to b27f6f5 Compare October 13, 2021 01:05
@mfisherlevine mfisherlevine changed the title Write util for calculating angle between wcses DM-32055: Write util for calculating angle between wcses Oct 13, 2021
Copy link
Contributor

@erykoff erykoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring needs to be updated for the new signature. Otherwise looks good. I'll click approve to save a button press later, but that needs to be updated. Other minor naming comments I leave up to you.

@@ -99,5 +101,47 @@ def skyToPixelArray(self, ra, dec, degrees=False):

return x.ravel(), y.ravel()

def getRelativeRotationAngleFromWcs(self, wcs):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I suggested the name off the top of my head, but is Angle redundant? Leave it up to you if you think getRelativeRotationFromWcs() sounds okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is now getRelativeRotationToWcs() for anyone following.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it "to" or "from"? There is a minus sign difference implied between the two names. It's worth thinking about which one is correct here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the name (and the docs) precisely because of that difference, but it's also entirely possible that I got it wrong. Do you think it looks wrong, or were you just urging caution in general as they're imply different things (in which case I totally agree!)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was general caution. I didn't try to figure it out. If you thought about it, then that's probably good.

And I just now took the chance to think about it too, and I think this is right. It's the (counter-clockwise) rotation you need to apply to this one to get to the other one. So yeah, "to" seems like the right preposition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks! I think this is all the more important now that it's a method that lives on the wcs itself.

python/lsst/afw/geom/skyWcs.py Show resolved Hide resolved
python/lsst/afw/geom/skyWcs.py Outdated Show resolved Hide resolved
@mfisherlevine mfisherlevine force-pushed the tickets/DM-32055 branch 2 times, most recently from 0e6730f to 0a52c97 Compare October 13, 2021 22:54
@mfisherlevine mfisherlevine merged commit 9503da7 into master Oct 14, 2021
@mfisherlevine mfisherlevine deleted the tickets/DM-32055 branch October 14, 2021 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants