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-4956: Adapt SRD-based measurements of astrometric performance for validate_drp #5

Merged
merged 21 commits into from Feb 13, 2016

Conversation

wmwv
Copy link
Contributor

@wmwv wmwv commented Feb 9, 2016

Addition of astrometric validation according to SRD specifications.
Refactoring to support this use.
Added documentation.

Separate loadAndMatchData into loadAndMatchData + analyzeData

Improve plotting.  Clarify mag_RMS vs. delta_mag.
Split plotting and calculation functions for AM1.
Clean up plotting code.
Fix schema key lookup.
Load SRD specifications from srdSpec.
Make simple coordinate test pass.
Pass coordinate tests at non-zero Dec.
Update calcAMx functions to use correct RA, Dec avg.
When using arccos to calculate distances on a sphere, points with
very close (or identical) coordinates led to round-off errors which
led to taking arccos(x>1.0).
Using haversine is the better choice and resolves this problem.
Vincenty is unlikely to be needed for this application, as calculating
distances across to opposites of the sphere is not part of the current
scope.
Uses the Haversine formula to preserve accuracy at small angles.

Law of cosines approach doesn't work well for the typically very small
differences that we're look at here.
Copy link
Member

Choose a reason for hiding this comment

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

...that we're looking at here...

@TallJimbo
Copy link
Member

Only general comment is that I'm concerned by the amount of explicit angle-wrangling and coordinate manipulation that's going on in this code.

We introduced the Angle class here to deal with the former, but it looks like you can't really use it much since it's not NumPy-friendly. Hopefully someday AstroPy integration will give us a better option; should we maybe create a deferred ticket to deal with that when we can (maybe that's a question for @frossie - I don't know if she considers long-deferred tickets to be helpful or harmful).

As for coordinate manipulation, it'd be very nice if you could take stock of all the low-level operations on coordinates you need here and create an issue for putting that kind of code in to afw or sphgeom. Of course, once again AstroPy integration may ultimately solve that problem for us.

@wmwv
Copy link
Contributor Author

wmwv commented Feb 12, 2016

I find it personally helpful to create long-delayed tickets (with no due dates) to represent thoughts of things that are being pushed off as out of scope.

@wmwv
Copy link
Contributor Author

wmwv commented Feb 12, 2016

I will think about the coordinate manipulations here and think about requests for base functions to implement these. 'averageCoord' was certainly helpful and one step in this direction. However, it's becoming clear to me that I think it should have been a spherical geometry routine, not a coordinate routine.

My current overall comment is that there a number of spherical geometry calculations that are instead in lsst.afw.coord. I shouldn't have to even known about the existence of ICRS to take the difference between two positions on the sphere.

    angleRa = [afwGeom.Angle(r, afwGeom.radians) for r in ra]
    angleDec = [afwGeom.Angle(d, afwGeom.radians) for d in dec]
    coords = [afwCoord.IcrsCoord(ar, ad) for (ar, ad) in zip(angleRa, angleDec)]

    meanRa, meanDec = afwCoord.averageCoord(coords)

wmwv added a commit that referenced this pull request Feb 13, 2016
DM-4956: Adapt SRD-based measurements of astrometric performance for validate_drp
@wmwv wmwv merged commit 68fde40 into master Feb 13, 2016
@ktlim ktlim deleted the tickets/DM-4956 branch August 25, 2018 06:50
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