Details
-
Type:
Improvement
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: Validation
-
Labels:None
-
Story Points:1
-
Epic Link:
-
Team:SQuaRE
Description
validate_drp would be easier to follow and safer if it took advantage of lsst.afw.geom.Angle and lsst.afw.coord.IcrsCoord. For instance averageRaDecFromCat could return an IcrsCoord and positionRms could use coord1.angularSeparation(coord2) and handle wraparound and other effects simply and safely.
Attachments
Activity
I'm afraid it may take me a few days (perhaps even after JTM) before I can review this.
The changes I saw on the pull request look useful but don't seem to have anything to do with this ticket. I didn't see any use of Angle or Coord/SpherePoint. I still think there are plenty of places where they would be valuable.
One of the changes does raise the question of whether Coord and/or SpherePoint should support vectorized versions of separation and such – at least in Python (such loops are trivial to write in C++).
So I don't really know what to make of this pull request. Sorry.
This pull request conceptually address part of the ticket by the previously wrong behavior of positionRms by now using sphDist to handle wrapping correctly. I realize that it does not, in fact, use afwCoord at all. Sorry for the confusion: half of my brain thinks that sphDist using afwCoord, the other half thinks it doesn't.
If performance of these coordinate calculation become a limiting issue for validate_drp I will take another look at this. In general, I'm happy to use C++ when necessary for performance gains, but I'm happier to keep calculations in Python unless necessary. If vectorized versions of afwCoord.angularSeparation and SpherePoint.separation become exposed through the Python interface then I'd be happy to look more generally at those aspects.
Michael Wood-Vasey
added a comment - This pull request conceptually address part of the ticket by the previously wrong behavior of positionRms by now using sphDist to handle wrapping correctly. I realize that it does not, in fact, use afwCoord at all. Sorry for the confusion: half of my brain thinks that sphDist using afwCoord , the other half thinks it doesn't.
If performance of these coordinate calculation become a limiting issue for validate_drp I will take another look at this. In general, I'm happy to use C++ when necessary for performance gains, but I'm happier to keep calculations in Python unless necessary. If vectorized versions of afwCoord.angularSeparation and SpherePoint.separation become exposed through the Python interface then I'd be happy to look more generally at those aspects.
While the incorrect code has been addressed, I appreciate that I have not responded to the main spirit of your request.
But I'm going to accept the pull request and I'm going to close this ticket to recognize the effort an improvements made for now. If performance concerns arise in the future where the use of C++ seems warranted, I will keep these suggestions in mind.
Michael Wood-Vasey
added a comment - While the incorrect code has been addressed, I appreciate that I have not responded to the main spirit of your request.
But I'm going to accept the pull request and I'm going to close this ticket to recognize the effort an improvements made for now. If performance concerns arise in the future where the use of C++ seems warranted, I will keep these suggestions in mind. 
Quick review.
I implemented the minimal version of this ticket. I updated positionRMS to use sphDist and thus afwCoord routines. It now correctly handles wrap-around and poles.
I did not change the underlying bookkeeping over to Coordinates, because these are not the fundamental objects stored in the output catalog schema or what MultiMatch uses. I do like the idea of being consistent, but I'm afraid that's not possible right now.