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-11162: Replace all use of Coord and subclasses with SpherePoint #12

Merged
merged 2 commits into from Mar 23, 2018

Conversation

r-owen
Copy link
Contributor

@r-owen r-owen commented Mar 16, 2018

No description provided.

.sconf_temp
config.log

Choose a reason for hiding this comment

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

Is there a reason to change where config.log appears in the file?

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 prefer having the .scons* files together

@@ -136,7 +136,7 @@ def source_record_to_value_list(self, source_record):

Parameters
----------
source_record : afw.table.SourceRecord
source_record : lsst.afw.table.SourceRecord

Choose a reason for hiding this comment

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

Not related to the Coord -> SpherePoint conversion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Are you requesting that it be on a separate commit?

Choose a reason for hiding this comment

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

I'm fine with it, but do we have a policy on this? I've been asked by other reviewers to do cleanups in separate commits, but I don't know if this is standard practice.

Copy link
Member

Choose a reason for hiding this comment

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

it is standard practice.

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'll just drop that change. I have a version of this file whose docs are cleaned up, but I don't want to mess up the merge of DM-13478 so I'll just hold on to it privately for now.

Copy link
Contributor

@mrawls mrawls left a comment

Choose a reason for hiding this comment

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

This looks fine. I am OK with your small reorganizations and cleanups being in this commit; these things will never happen if they aren't done when they come up.

@@ -90,13 +89,11 @@ def edit_and_offset_source_record(src, src_id, ra_degrees, dec_degrees,
scatter_arcsec : float
Arcsecond scatter to add to the position of the source record coord.
"""
coord = Coord(afwGeom.Angle(ra_degrees, units=afwGeom.degrees),
afwGeom.Angle(dec_degrees, units=afwGeom.degrees))
coord = lsst.afw.geom.SpherePoint(ra_degrees, dec_degrees, afwGeom.degrees)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a lovely example of how SpherePoint makes our lives better! 💯

@r-owen r-owen merged commit 4768d6a into master Mar 23, 2018
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