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-14494: selectImages.py depends on defunct lsst.geom #20

Merged
merged 3 commits into from May 17, 2018

Conversation

kfindeisen
Copy link
Member

This PR fixes an ImportError in selectImages.py that propagates to imports of lsst.ap.pipe. The __init__ file for ap.pipe may also be edited as part of this PR, but I'm waiting for confirmation from @ebellm that it's safe to do so.

lsst.geom no longer exists; the replacement for convexHull is
sphgeom.ConvexPolygon.convexHull.
@kfindeisen kfindeisen requested a review from mrawls May 17, 2018 17:54
Copy link
Collaborator

@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.

Looks fine to me.

We use "convexHull" instead of generating a SphericalConvexPolygon
directly because the standard for the inputs to SphericalConvexPolygon
are pretty high and we don't want to be responsible for reaching them.
If "convexHull" is found to be too slow, we can revise this.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't entirely understand the context for this original comment, but given that the entirety of selectImages.py will be removed as part of DM-11953, I'm not overly concerned about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

lsst.geom had a "with input checks" and a "without checks" way to get a convex polygon. lsst.sphgeom always checks, as far as I can tell.

@kfindeisen kfindeisen merged commit 92cb7d5 into master May 17, 2018
@kfindeisen kfindeisen deleted the tickets/DM-14494 branch February 25, 2019 19:51
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

2 participants