Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-5159

Please use angle and Coord where possible

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: Validation
    • Labels:
      None

      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

          Hide
          wmwood-vasey Michael Wood-Vasey added a comment -

          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.

          Show
          wmwood-vasey Michael Wood-Vasey added a comment - 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.
          Hide
          rowen Russell Owen added a comment -

          I'm afraid it may take me a few days (perhaps even after JTM) before I can review this.

          Show
          rowen Russell Owen added a comment - I'm afraid it may take me a few days (perhaps even after JTM) before I can review this.
          Hide
          rowen Russell Owen added a comment -

          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.

          Show
          rowen Russell Owen added a comment - 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.
          Hide
          wmwood-vasey 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.

          Show
          wmwood-vasey 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.
          Hide
          wmwood-vasey 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.

          Show
          wmwood-vasey 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.

            People

            • Assignee:
              wmwood-vasey Michael Wood-Vasey
              Reporter:
              rowen Russell Owen
              Reviewers:
              Russell Owen
              Watchers:
              Krzysztof Findeisen, Michael Wood-Vasey, Russell Owen
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: