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-20566: Replace afwGeom with geom and remove py2.x support #23

Merged
merged 2 commits into from Jul 19, 2019

Conversation

timj
Copy link
Member

@timj timj commented Jul 18, 2019

No description provided.

Copy link
Member

@isullivan isullivan left a comment

Choose a reason for hiding this comment

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

Very minor comments: there is some inconsistency in the docstrings (e.g. lsst.geom.Angle vs geom.Angle), and some of the python packages have extra whitespace around * operators. I don't think you have to fix either, though.

@@ -36,8 +36,8 @@ def main():
indexid = ids[0]

idName = 'id'
X = solver.getCatalogue(ra * afwGeom.degrees, dec * afwGeom.degrees,
radius * afwGeom.degrees, '', idName, indexid)
X = solver.getCatalogue(ra * geom.degrees, dec * geom.degrees,
Copy link
Member

Choose a reason for hiding this comment

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

You might also remove the spaces around the * operators, but I recognize that that would make the style inconsistent with the rest of the file.

@@ -300,7 +300,7 @@ def _getImageParams(self, exposure=None, bbox=None, wcs=None, filterName=None, w
"""Get image parameters

@param[in] exposure exposure (an afwImage.Exposure) or None
@param[in] bbox bounding box (an afwGeom.Box2I) or None; if None then bbox must be specified
@param[in] bbox bounding box (an geom.Box2I) or None; if None then bbox must be specified
Copy link
Member

Choose a reason for hiding this comment

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

Should this be lsst.geom.Box2I?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but since (1) these docstrings all need fixing up when they are converted to numpydoc and (2) they already do it wrong by using afwImage in the line above, I think I'm better off not fixing these on this pass.

Copy link
Member

Choose a reason for hiding this comment

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

Not a problem. Leave these as they are, then.

@@ -435,7 +435,7 @@ def determineWcs(self, sourceCat, exposure, **kwargs):
controls whether we do that or force Astrometry.net to
search both parities. Default from config.useWcsParity.

'pixelScale': afwGeom.Angle, estimate of the angle-per-pixel
'pixelScale': geom.Angle, estimate of the angle-per-pixel
Copy link
Member

Choose a reason for hiding this comment

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

Should this be lsst.geom.Angle?

@@ -456,7 +456,7 @@ def determineWcs(self, sourceCat, exposure, **kwargs):

margs = kwargs.copy()
if 'searchRadius' not in margs:
margs.update(searchRadius=self.config.raDecSearchRadius * afwGeom.degrees)
margs.update(searchRadius=self.config.raDecSearchRadius * geom.degrees)
Copy link
Member

Choose a reason for hiding this comment

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

Consider removing whitespace around the operator.

return pipeBase.Struct(
distMean=distMean,
distStdDev=distStdDev,
maxMatchDist=distMean + self.config.matchDistanceSigma*distStdDev,
)

def _getMatchList(self, sourceCat, refCat, wcs):
dist = self.config.catalogMatchDist * afwGeom.arcseconds
dist = self.config.catalogMatchDist * geom.arcseconds
Copy link
Member

Choose a reason for hiding this comment

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

Consider removing whitespace around the operator.

@@ -86,7 +86,7 @@ def loadSkyCircle(self, ctrCoord, radius, filterName=None, epoch=None, centroids
"""!Load reference objects that overlap a circular sky region

@param[in] ctrCoord center of search region (an afwGeom.Coord)
@param[in] radius radius of search region (an afwGeom.Angle)
@param[in] radius radius of search region (an geom.Angle)
Copy link
Member

Choose a reason for hiding this comment

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

lsst.geom.Angle?

@@ -188,7 +188,7 @@ def _getMIndexesWithinRange(self, ctrCoord, radius):
"""!Get list of muti-index objects within range

@param[in] ctrCoord center of search region (an afwGeom.Coord)
@param[in] radius radius of search region (an afwGeom.Angle)
@param[in] radius radius of search region (an geom.Angle)
Copy link
Member

Choose a reason for hiding this comment

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

lsst.geom.Angle?

@timj
Copy link
Member Author

timj commented Jul 19, 2019

Thanks, I think I'd like to avoid going in and "fixing" all the * operator usage since that will have to be done consistently in the package for it to be worthwhile.

@isullivan
Copy link
Member

Yes, don't bother changing the whitespace around the * operators on this ticket.

@timj timj merged commit 31c43f9 into master Jul 19, 2019
@timj timj deleted the tickets/DM-20566-meas_extensions_astrometryNet branch July 19, 2019 21:49
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