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-13891: matchRaDec: fix imprecision at small distance #335
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I requested some changes in the unit test.
tests/test_sourceMatch.py
Outdated
|
||
Based on DM-13891. | ||
""" | ||
rng = np.random.RandomState(12345) # I have the same combination on my luggage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cute
tests/test_sourceMatch.py
Outdated
radius = 0.5*afwGeom.arcseconds | ||
num = 1000 | ||
raKey = afwTable.SourceTable.getCoordKey().getRa() | ||
decKey = afwTable.SourceTable.getCoordKey().getDec() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These aren't used below, are they? (Please always run a flake8 linter)
tests/test_sourceMatch.py
Outdated
coord = src1.getCoord().offset(rng.uniform(high=360)*afwGeom.degrees, | ||
rng.uniform(high=radius.asArcseconds())*afwGeom.arcseconds) | ||
src2.set(afwTable.SourceTable.getCoordKey().getRa(), coord.getLongitude()) | ||
src2.set(afwTable.SourceTable.getCoordKey().getDec(), coord.getLatitude()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't this be written as src2.set(coordKey, coord)
where you get the coordKey outside the loop?
I had no idea that getCoordKey().getRa() would return a key for the RA field. Interesting.
matches = afwTable.matchRaDec(self.ss1, self.ss2, radius) | ||
dist1 = np.array([(mm.distance*afwGeom.radians).asArcseconds() for mm in matches]) | ||
dist2 = np.array([mm.first.getCoord().separation(mm.second.getCoord()).asArcseconds() for mm in matches]) | ||
diff = dist1 - dist2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of comparing a summary statistic I strongly suggest comparing values individually, e.g. by calling self.assertFloatsAlmostEqual
or numpy.testing.assert_allclose
898827b
to
b4dcf4f
Compare
The formula used for converting unit sphere distance^2 to an Angle isn't precise for small distance. Using an alternative formula that (unfortunately) includes a sqrt is more precise. Bug identified by Dominique Boutigny.
b4dcf4f
to
ce0a03c
Compare
The formula used for converting unit sphere distance^2 to an Angle
isn't precise for small distance. Using an alternative formula that
(unfortunately) includes a sqrt is more precise.
Bug identified by Dominique Boutigny.