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

Replace all use of Coord and subclasses with SpherePoint

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Story Points:
      20
    • Epic Link:
    • Sprint:
      Alert Production F17 - 8, Alert Production F17 - 9, AP S18-2, AP S18-4
    • Team:
      Alert Production

      Description

      Implement RFC-353 by replacing all use of Coord and its subclasses with SpherePoint.

      Things to keep in mind:

      • afw will no longer support coordinate conversions. We do not rely on this now (except for a very small number of cases of use of FK5 J2000 instead if ICRS that is likely a mistake).
      • SpherePoint and Coord have slightly different APIs (for instance SpherePoint is immutable) so some usage will need to change.
      • Coord has some methods that SpherePoint does not. These may have to be added to SpherePoint or implemented as free functions.

      This would also be a good time to eliminate the SpherePoint(double raDecRad[2]) constructor. This was added to support Transform but turned out to be unnecessary.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment -

            DM-10765 has made the circular import problem for SkyWcs worse. I giving up on having SkyWcs imported into lsst.afw.geom by default; until this ticket is implemented users will have to import SkyWcs and the other symbols defined in skyWcs using something like from lsst.afw.geom.skyWcs import SkyWcs. As part of implementing this ticket, look for such imports and simplify them, and update Python documentation that refers to lsst.afw.geom.skyWcs.SkyWcs.

            Show
            rowen Russell Owen added a comment - DM-10765 has made the circular import problem for SkyWcs worse. I giving up on having SkyWcs imported into lsst.afw.geom by default; until this ticket is implemented users will have to import SkyWcs and the other symbols defined in skyWcs using something like from lsst.afw.geom.skyWcs import SkyWcs . As part of implementing this ticket, look for such imports and simplify them, and update Python documentation that refers to lsst.afw.geom.skyWcs.SkyWcs .
            Hide
            rowen Russell Owen added a comment - - edited

            The changes are described in this community posting: https://community.lsst.org/t/lsst-afw-coord-coord-classes-are-going-away/2769

            Most of the packages I had to change have trivial, obvious changes. afw, naturally, needed a lot of changes. Also obs_lsstSim has one non-obvious change: the camera mapper has special logic in std_raw to handle old raw images which used coordinate conversion capabilities of the Coord classes that are gone. I reworked the code (greatly simplifying it). The results are slightly, but not significantly, different.

            Show
            rowen Russell Owen added a comment - - edited The changes are described in this community posting: https://community.lsst.org/t/lsst-afw-coord-coord-classes-are-going-away/2769 Most of the packages I had to change have trivial, obvious changes. afw, naturally, needed a lot of changes. Also obs_lsstSim has one non-obvious change: the camera mapper has special logic in std_raw to handle old raw images which used coordinate conversion capabilities of the Coord classes that are gone. I reworked the code (greatly simplifying it). The results are slightly, but not significantly, different.
            Hide
            rowen Russell Owen added a comment -

            Meredith Rawls please review the changes to the three ap packages: ap_association, ap_pipe and ap_verify

            Show
            rowen Russell Owen added a comment - Meredith Rawls please review the changes to the three ap packages: ap_association, ap_pipe and ap_verify
            Hide
            rowen Russell Owen added a comment -

            Bob Armstrong heroically reviewed all the packages except pipe_analysis. Since that is not a DM package I am leaving it unmerged for now and merging the rest.

            @yusra I will leave it to folks at Princeton to review pipe_analysis when they have time. I am happy to respond to reviewer comments. The changes for this ticket are small, but I also fixed all the pep8 violations, which was a big change (and a separate commit).

            Show
            rowen Russell Owen added a comment - Bob Armstrong heroically reviewed all the packages except pipe_analysis. Since that is not a DM package I am leaving it unmerged for now and merging the rest. @yusra I will leave it to folks at Princeton to review pipe_analysis when they have time. I am happy to respond to reviewer comments. The changes for this ticket are small, but I also fixed all the pep8 violations, which was a big change (and a separate commit).
            Show
            lauren Lauren MacArthur added a comment - Just to follow up...these changes have now been merged to master on pipe_analysis , in particular: https://github.com/lsst-dm/pipe_analysis/commit/233ef51dca766f1b07c62eb613fef31f75253f0e https://github.com/lsst-dm/pipe_analysis/commit/d1896a66d2319001a39b2029f700fc56caf3976a https://github.com/lsst-dm/pipe_analysis/commit/17aec3d6d826cf7e2d7e41512c5bc92e9c48f229  Sorry it took so long!

              People

              • Assignee:
                rowen Russell Owen
                Reporter:
                rowen Russell Owen
                Reviewers:
                Bob Armstrong, Meredith Rawls
                Watchers:
                Bob Armstrong, Colin Slater, Jim Bosch, John Swinbank, Krzysztof Findeisen, Lauren MacArthur, Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                7 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel