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

Replace existing WCS classes with SkyWcs

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Story Points:
      27
    • Epic Link:
    • Sprint:
      Alert Production S17 - 6, Alert Production F17 - 7, Alert Production F17 - 8, Alert Production F17 - 9, Alert Production F17 - 10, Alert Production F17 - 11, AP S18-1, AP S18-2, AP S18-3
    • Team:
      Alert Production

      Description

      Replace all use of lsst.afw.image.Wcs and subclasses with lsst.afw.geom.SkyWcs in the DM stack.

      This will also require replacing much of our use of XYTransform with TransformPoint2ToPoint2

      The changes are described in this community post: Replacing Wcs and TanWcs with SkyWcs

        Attachments

          Issue Links

            Activity

            rowen Russell Owen created issue -
            rowen Russell Owen made changes -
            Field Original Value New Value
            Epic Link DM-9679 [ 30784 ]
            rowen Russell Owen made changes -
            Link This issue is blocked by DM-11052 [ DM-11052 ]
            krughoff Simon Krughoff made changes -
            Sprint Alert Production S17 - 6 [ 616 ] Alert Production S17 - 6, Alert Production F17 - 7 [ 616, 626 ]
            krughoff Simon Krughoff made changes -
            Rank Ranked higher
            ebellm Eric Bellm made changes -
            Sprint Alert Production S17 - 6, Alert Production F17 - 7 [ 616, 626 ] Alert Production S17 - 6, Alert Production F17 - 7, Alert Production F17 - 8 [ 616, 626, 632 ]
            ebellm Eric Bellm made changes -
            Rank Ranked higher
            rowen Russell Owen made changes -
            Link This issue blocks DM-11162 [ DM-11162 ]
            rowen Russell Owen made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-5922 [ DM-5922 ]
            rowen Russell Owen made changes -
            Link This issue is blocked by DM-5922 [ DM-5922 ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-5922 [ DM-5922 ]
            swinbank John Swinbank made changes -
            Sprint Alert Production S17 - 6, Alert Production F17 - 7, Alert Production F17 - 8 [ 616, 626, 632 ] Alert Production S17 - 6, Alert Production F17 - 7, Alert Production F17 - 8, Alert Production F17 - 9 [ 616, 626, 632, 639 ]
            swinbank John Swinbank made changes -
            Rank Ranked higher
            rowen Russell Owen made changes -
            Link This issue relates to DM-11895 [ DM-11895 ]
            rowen Russell Owen made changes -
            Story Points 30 27
            rowen Russell Owen made changes -
            Link This issue is blocked by DM-11895 [ DM-11895 ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-11895 [ DM-11895 ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-10411 [ DM-10411 ]
            rowen Russell Owen made changes -
            Link This issue is blocked by DM-11957 [ DM-11957 ]
            swinbank John Swinbank made changes -
            Sprint Alert Production S17 - 6, Alert Production F17 - 7, Alert Production F17 - 8, Alert Production F17 - 9 [ 616, 626, 632, 639 ] Alert Production S17 - 6, Alert Production F17 - 7, Alert Production F17 - 8, Alert Production F17 - 9, Alert Production F17 - 10 [ 616, 626, 632, 639, 643 ]
            swinbank John Swinbank made changes -
            Rank Ranked higher
            rowen Russell Owen made changes -
            Link This issue is triggered by RFC-394 [ RFC-394 ]
            rowen Russell Owen made changes -
            Link This issue is blocked by DM-12230 [ DM-12230 ]
            rowen Russell Owen made changes -
            Link This issue is triggered by RFC-403 [ RFC-403 ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-12270 [ DM-12270 ]
            rowen Russell Owen made changes -
            Link This issue is blocked by DM-12272 [ DM-12272 ]
            rowen Russell Owen made changes -
            Link This issue is blocked by DM-12270 [ DM-12270 ]
            rowen Russell Owen made changes -
            Description Replace all use of {{lsst.afw.image.Wcs}} and subclasses with {{lsst.afw.geom.SkyWcs}} in the DM stack. Replace all use of {{lsst.afw.image.Wcs}} and subclasses with {{lsst.afw.geom.SkyWcs}} in the DM stack.

            This will also require replacing much of our use of XYTransform with TransformPoint2ToPoint2
            rowen Russell Owen made changes -
            Link This issue relates to DM-12452 [ DM-12452 ]
            swinbank John Swinbank made changes -
            Sprint Alert Production S17 - 6, Alert Production F17 - 7, Alert Production F17 - 8, Alert Production F17 - 9, Alert Production F17 - 10 [ 616, 626, 632, 639, 643 ] Alert Production S17 - 6, Alert Production F17 - 7, Alert Production F17 - 8, Alert Production F17 - 9, Alert Production F17 - 10, Alert Production F17 - 11 [ 616, 626, 632, 639, 643, 644 ]
            swinbank John Swinbank made changes -
            Rank Ranked higher
            rowen Russell Owen made changes -
            Link This issue relates to DM-12524 [ DM-12524 ]
            Parejkoj John Parejko made changes -
            Link This issue is blocked by DM-12690 [ DM-12690 ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-12764 [ DM-12764 ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-12771 [ DM-12771 ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-12771 [ DM-12771 ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-12773 [ DM-12773 ]
            swinbank John Swinbank made changes -
            Sprint Alert Production S17 - 6, Alert Production F17 - 7, Alert Production F17 - 8, Alert Production F17 - 9, Alert Production F17 - 10, Alert Production F17 - 11 [ 616, 626, 632, 639, 643, 644 ] Alert Production S17 - 6, Alert Production F17 - 7, Alert Production F17 - 8, Alert Production F17 - 9, Alert Production F17 - 10, Alert Production F17 - 11, AP S18-1 [ 616, 626, 632, 639, 643, 644, 669 ]
            swinbank John Swinbank made changes -
            Rank Ranked higher
            Hide
            jbosch Jim Bosch added a comment -

            This came to mind when thinking about the review for DM-12764, but it's not really a question for that issue:

            Are we planning to actually remove Wcs and/or rename SkyWcs to Wcs on this ticket?

            I think we should do it:

            • It's less disruptive (while we may have needed to make more changes during the transition, the number of changes actually produced by the merge and seen by people other than Russell Owen should go way down).
            • When someone asks me we use a certain name for a certain code object (say, "afw", or "pex"), I hate it when the only answer is "history", and that's what the answer will be for SkyWcs unless we anticipate there being other kinds of Wcs classes (and I don't think we do).

            It's obviously disruptive if we do the merge before we rename SkyWcs, and we can't do the rename until we remove Wcs, and to me that says we should do them both here (or at least on another ticket we merge back-to-back). I hope that isn't too inconsistent with the current plan.

            Show
            jbosch Jim Bosch added a comment - This came to mind when thinking about the review for DM-12764 , but it's not really a question for that issue: Are we planning to actually remove Wcs and/or rename SkyWcs to Wcs on this ticket? I think we should do it: It's less disruptive (while we may have needed to make more changes during the transition, the number of changes actually produced by the merge and seen by people other than Russell Owen should go way down). When someone asks me we use a certain name for a certain code object (say, "afw", or "pex"), I hate it when the only answer is "history", and that's what the answer will be for SkyWcs unless we anticipate there being other kinds of Wcs classes (and I don't think we do). It's obviously disruptive if we do the merge before we rename SkyWcs, and we can't do the rename until we remove Wcs, and to me that says we should do them both here (or at least on another ticket we merge back-to-back). I hope that isn't too inconsistent with the current plan.
            Hide
            tjenness Tim Jenness added a comment -

            In theory you will be interested in a SpectralWCS and a SkySpectralWCS. I know that there is no plan to use a proper WCS for the spectral reduction but that should be on the table.

            Show
            tjenness Tim Jenness added a comment - In theory you will be interested in a SpectralWCS and a SkySpectralWCS. I know that there is no plan to use a proper WCS for the spectral reduction but that should be on the table.
            Hide
            rowen Russell Owen added a comment -

            Jim Bosch I don't think renaming SkyWcs will help as much as you think, for these reasons:

            • The new code does not have a makeWcs function because I prefer calling a constructor for basic construction, as you saw on DM-12764. Thus there is a SkyWcs(metdata) constructor for the general FITS-WCS case, and SkyWcs(crpix, crval, cdMatrix, projection) will construct all the no-distortion WCS we use (there are some exotic cases it does not cover, but the metadata constructor works for those and so far we don't use them anyway). For TAN-SIP I do offer makeTanSipWcs, since that seems a bit specialized.
            • Note that there are some differences between SkyWcs(crpix, crval, cdMatrix, projection) and makeWcs: cdMatrix is a single argument, typically constructed with the new function makeCdMatrix, and I swapped the first two arguments (since the underlying Transform is from pixels to sky). When I modified the Python code I used named keyword arguments to avoid confusion.
            • SkyWcs is in lsst.afw.geom, where I think it belongs, not lsst.afw.image. Admittedly this does mean one has to go one level deeper in Python, due to a circular dependency with lsst.afw.table. That reminds me: I think you wanted to add table persistence to Transform; if you do that then you are going to introduce the same problem there, which will be somewhat disruptive. It may be worth thinking a bit more about a solution to this problem.
            • Many packages needed small changes, partly due to unit tests, and those changes have been made.

            As Tim Jenness says, I picked the name on purpose to suggest a 2-dimensional celestial WCS, in case we decide to add support for other kinds of WCS in the future.

            Show
            rowen Russell Owen added a comment - Jim Bosch I don't think renaming SkyWcs will help as much as you think, for these reasons: The new code does not have a makeWcs function because I prefer calling a constructor for basic construction, as you saw on DM-12764 . Thus there is a SkyWcs(metdata) constructor for the general FITS-WCS case, and SkyWcs(crpix, crval, cdMatrix, projection) will construct all the no-distortion WCS we use (there are some exotic cases it does not cover, but the metadata constructor works for those and so far we don't use them anyway). For TAN-SIP I do offer makeTanSipWcs , since that seems a bit specialized. Note that there are some differences between SkyWcs(crpix, crval, cdMatrix, projection) and makeWcs: cdMatrix is a single argument, typically constructed with the new function makeCdMatrix, and I swapped the first two arguments (since the underlying Transform is from pixels to sky). When I modified the Python code I used named keyword arguments to avoid confusion. SkyWcs is in lsst.afw.geom, where I think it belongs, not lsst.afw.image. Admittedly this does mean one has to go one level deeper in Python, due to a circular dependency with lsst.afw.table. That reminds me: I think you wanted to add table persistence to Transform; if you do that then you are going to introduce the same problem there, which will be somewhat disruptive. It may be worth thinking a bit more about a solution to this problem. Many packages needed small changes, partly due to unit tests, and those changes have been made. As Tim Jenness says, I picked the name on purpose to suggest a 2-dimensional celestial WCS, in case we decide to add support for other kinds of WCS in the future.
            Hide
            rowen Russell Owen added a comment - - edited

            DM-10765 is getting close to ready to merge. The new APIs in afw have stabilized and I’d like to get the reviews started. John Swinbank volunteered you three, and I hope you will be able to do it.

            afw has the most changes, followed by meas_algorithms and meas_astrom. ip_isr adds a method to IsrTask that adds a model for optical distortion (based on camera geometry) to the WCS. The assignments are fairly arbitrary, so please feel free to swap around or complain if you feel you got too much work.

            Lauren: please review meas_algorithms, the obs packages, skymap and ip_isr

            Josh: please review meas_base, meas_astrom, skypix, meas_extensions_astrometryNet, pipe_tasks, coadd_utils, coadd_chisquared, meas_modelfit and meas_mosaic.

            Bob: please review afw, astshim, ip_diffIm, meas_modelfit, meas_extensions_covolved, meas_extensions_photometryKron, meas_extensions_psfex and meas_extensions_shapeHSM

            Show
            rowen Russell Owen added a comment - - edited DM-10765 is getting close to ready to merge. The new APIs in afw have stabilized and I’d like to get the reviews started. John Swinbank volunteered you three, and I hope you will be able to do it. afw has the most changes, followed by meas_algorithms and meas_astrom. ip_isr adds a method to IsrTask that adds a model for optical distortion (based on camera geometry) to the WCS. The assignments are fairly arbitrary, so please feel free to swap around or complain if you feel you got too much work. Lauren: please review meas_algorithms, the obs packages, skymap and ip_isr Josh: please review meas_base, meas_astrom, skypix, meas_extensions_astrometryNet, pipe_tasks, coadd_utils, coadd_chisquared, meas_modelfit and meas_mosaic. Bob: please review afw, astshim, ip_diffIm, meas_modelfit, meas_extensions_covolved, meas_extensions_photometryKron, meas_extensions_psfex and meas_extensions_shapeHSM
            rowen Russell Owen made changes -
            Reviewers Bob Armstrong, John Parejko, Josh Frieman, Lauren MacArthur [ rearmstr, parejkoj, frieman, lauren ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            rowen Russell Owen added a comment -

            Also, John Parejko please review the changes in jointcal.

            Show
            rowen Russell Owen added a comment - Also, John Parejko please review the changes in jointcal.
            Hide
            jbosch Jim Bosch added a comment -

            Russell Owen, could you do a stack-wide git tag before this merge, or ping Paul Price or me just before you do? We're trying to get an HSC stable release out soon, and I think we'll want to branch before these changes land (without actually holding up your merge).

            Show
            jbosch Jim Bosch added a comment - Russell Owen , could you do a stack-wide git tag before this merge, or ping Paul Price or me just before you do? We're trying to get an HSC stable release out soon, and I think we'll want to branch before these changes land (without actually holding up your merge).
            rowen Russell Owen made changes -
            Link This issue is blocked by DM-12764 [ DM-12764 ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-12764 [ DM-12764 ]
            Parejkoj John Parejko made changes -
            Reviewers Bob Armstrong, John Parejko, Josh Frieman, Lauren MacArthur [ rearmstr, parejkoj, frieman, lauren ] Bob Armstrong, Josh Frieman, Lauren MacArthur [ rearmstr, frieman, lauren ]
            Hide
            rowen Russell Owen added a comment -

            John Parejko thank you for the helpful review. I made your suggested changes and also changed GtransfoSkyWcs to hold its SkyWcs by shared_ptr. Might you have another quick look as a sanity-check? In particular I would like to make sure you are happy with the new GransfoSkyWcs class description.

            Show
            rowen Russell Owen added a comment - John Parejko thank you for the helpful review. I made your suggested changes and also changed GtransfoSkyWcs to hold its SkyWcs by shared_ptr. Might you have another quick look as a sanity-check? In particular I would like to make sure you are happy with the new GransfoSkyWcs class description.
            Hide
            Parejkoj John Parejko added a comment -

            I'm happy.

            Show
            Parejkoj John Parejko added a comment - I'm happy.
            jbosch Jim Bosch made changes -
            Link This issue is blocked by DM-13065 [ DM-13065 ]
            rowen Russell Owen made changes -
            Link This issue relates to RFC-427 [ RFC-427 ]
            rowen Russell Owen made changes -
            Link This issue is triggered by RFC-427 [ RFC-427 ]
            rowen Russell Owen made changes -
            Link This issue relates to RFC-427 [ RFC-427 ]
            rowen Russell Owen made changes -
            Description Replace all use of {{lsst.afw.image.Wcs}} and subclasses with {{lsst.afw.geom.SkyWcs}} in the DM stack.

            This will also require replacing much of our use of XYTransform with TransformPoint2ToPoint2
            Replace all use of {{lsst.afw.image.Wcs}} and subclasses with {{lsst.afw.geom.SkyWcs}} in the DM stack.

            This will also require replacing much of our use of XYTransform with TransformPoint2ToPoint2

            The changes are described in this community post: https://community.lsst.org/t/replacing-wcs-and-tanwcs-with-skywcs/2531
            rowen Russell Owen made changes -
            Description Replace all use of {{lsst.afw.image.Wcs}} and subclasses with {{lsst.afw.geom.SkyWcs}} in the DM stack.

            This will also require replacing much of our use of XYTransform with TransformPoint2ToPoint2

            The changes are described in this community post: https://community.lsst.org/t/replacing-wcs-and-tanwcs-with-skywcs/2531
            Replace all use of {{lsst.afw.image.Wcs}} and subclasses with {{lsst.afw.geom.SkyWcs}} in the DM stack.

            This will also require replacing much of our use of XYTransform with TransformPoint2ToPoint2

            The changes are described in this community post: [Replacing Wcs and TanWcs with SkyWcs|https://community.lsst.org/t/replacing-wcs-and-tanwcs-with-skywcs/2531]
            rowen Russell Owen made changes -
            Link This issue is blocked by DM-13166 [ DM-13166 ]
            rowen Russell Owen made changes -
            Reviewers Bob Armstrong, Josh Frieman, Lauren MacArthur [ rearmstr, frieman, lauren ] Bob Armstrong, Lauren MacArthur [ rearmstr, lauren ]
            rowen Russell Owen made changes -
            Watchers Bob Armstrong, Jim Bosch, John Parejko, Josh Frieman, Lauren MacArthur, Russell Owen, Tim Jenness [ Bob Armstrong, Jim Bosch, John Parejko, Josh Frieman, Lauren MacArthur, Russell Owen, Tim Jenness ] Bob Armstrong, Jim Bosch, John Parejko, Lauren MacArthur, Russell Owen, Tim Jenness [ Bob Armstrong, Jim Bosch, John Parejko, Lauren MacArthur, Russell Owen, Tim Jenness ]
            rowen Russell Owen made changes -
            Reviewers Bob Armstrong, Lauren MacArthur [ rearmstr, lauren ] Bob Armstrong, Joshua Meyers, Lauren MacArthur [ rearmstr, jmeyers314, lauren ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-13065 [ DM-13065 ]
            rowen Russell Owen made changes -
            Link This issue is blocked by DM-13065 [ DM-13065 ]
            swinbank John Swinbank made changes -
            Sprint Alert Production S17 - 6, Alert Production F17 - 7, Alert Production F17 - 8, Alert Production F17 - 9, Alert Production F17 - 10, Alert Production F17 - 11, AP S18-1 [ 616, 626, 632, 639, 643, 644, 669 ] Alert Production S17 - 6, Alert Production F17 - 7, Alert Production F17 - 8, Alert Production F17 - 9, Alert Production F17 - 10, Alert Production F17 - 11, AP S18-1, AP S18-2 [ 616, 626, 632, 639, 643, 644, 669, 677 ]
            swinbank John Swinbank made changes -
            Rank Ranked higher
            Parejkoj John Parejko made changes -
            Link This issue relates to DM-13187 [ DM-13187 ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-13201 [ DM-13201 ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-13201 [ DM-13201 ]
            Hide
            rowen Russell Owen added a comment -

            John Parejko please have another look at jointcal; I had to make a few minor changes due to your recent changes on master.

            Show
            rowen Russell Owen added a comment - John Parejko please have another look at jointcal; I had to make a few minor changes due to your recent changes on master.
            Hide
            Parejkoj John Parejko added a comment -

            Thank you: I continue to be happy.

            Show
            Parejkoj John Parejko added a comment - Thank you: I continue to be happy.
            Hide
            rearmstr Bob Armstrong added a comment -

            I've looked over my assigned packages and made some minor comments on github. Looks good!

            Show
            rearmstr Bob Armstrong added a comment - I've looked over my assigned packages and made some minor comments on github. Looks good!
            rearmstr Bob Armstrong made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            Hide
            rearmstr Bob Armstrong added a comment -

            I prematurely marked this as reviewed. It is still waiting on Josh.

            Show
            rearmstr Bob Armstrong added a comment - I prematurely marked this as reviewed. It is still waiting on Josh.
            rearmstr Bob Armstrong made changes -
            Status Reviewed [ 10101 ] In Review [ 10004 ]
            Hide
            jmeyers314 Joshua Meyers added a comment - - edited

            Looks good.  I had a few small comments (on GitHub) on my assigned packages.

            Show
            jmeyers314 Joshua Meyers added a comment - - edited Looks good.  I had a few small comments (on GitHub) on my assigned packages.
            Hide
            lauren Lauren MacArthur added a comment -

            Ok, I'm done.  Mostly looks good, but see my comments on the PRs (and note that I ignored PR #97 as it seems to belong to another ticket).  I am assuming there will be thorough testing (Jenkins and a RC reduction run, I hear) before this is merged.

            Show
            lauren Lauren MacArthur added a comment - Ok, I'm done.  Mostly looks good, but see my comments on the PRs (and note that I ignored PR #97 as it seems to belong to another ticket).  I am assuming there will be thorough testing (Jenkins and a RC reduction run, I hear) before this is merged.
            rowen Russell Owen made changes -
            Link This issue blocks DM-13381 [ DM-13381 ]
            rowen Russell Owen made changes -
            Link This issue is blocked by DM-13316 [ DM-13316 ]
            swinbank John Swinbank made changes -
            Sprint Alert Production S17 - 6, Alert Production F17 - 7, Alert Production F17 - 8, Alert Production F17 - 9, Alert Production F17 - 10, Alert Production F17 - 11, AP S18-1, AP S18-2 [ 616, 626, 632, 639, 643, 644, 669, 677 ] Alert Production S17 - 6, Alert Production F17 - 7, Alert Production F17 - 8, Alert Production F17 - 9, Alert Production F17 - 10, Alert Production F17 - 11, AP S18-1, AP S18-2, AP S18-3 [ 616, 626, 632, 639, 643, 644, 669, 677, 683 ]
            rowen Russell Owen made changes -
            Link This issue is blocked by DM-13539 [ DM-13539 ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-13554 [ DM-13554 ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-13564 [ DM-13564 ]
            rowen Russell Owen made changes -
            Resolution Done [ 10000 ]
            Status In Review [ 10004 ] Done [ 10002 ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-13680 [ DM-13680 ]
            swinbank John Swinbank made changes -
            Link This issue relates to DM-197 [ DM-197 ]
            swinbank John Swinbank made changes -
            Link This issue is duplicated by DM-1971 [ DM-1971 ]
            swinbank John Swinbank made changes -
            Link This issue is duplicated by DM-9889 [ DM-9889 ]

              People

              • Assignee:
                rowen Russell Owen
                Reporter:
                rowen Russell Owen
                Reviewers:
                Bob Armstrong, Joshua Meyers, Lauren MacArthur
                Watchers:
                Bob Armstrong, Jim Bosch, John Parejko, Joshua Meyers, Lauren MacArthur, Russell Owen, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                7 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel