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

Create new Wcs class

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Story Points:
      20
    • Epic Link:
    • Sprint:
      Alert Production S17 - 4, Alert Production S17 - 5
    • Team:
      Alert Production

      Description

      Create the new flexible AST-backed WCS class.

      This will be called lsst::afw:;geom::SkyWcs to indicate that it is a celestial WCS. It will implement most of the methods of the existing lsst::afw::image::Wcs. It will subclass from lsst::afw::geom::Transform<Point2Endpoint, SpherePointEndpoint>, because that class already performs the necessary pixel-to-sky transformations. It will include various standard frames to support the LSST pixel convention.

      Note that this ticket is purely to create the new WCS class. It is not intended to replace usage of the existing WCS class.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment - - edited

            I made some changes to astshim as part of this, to make it easier for Transform and Wcs:

            • Add a copy argument to FrameSet.getFrame with a default value of true. Being able to access a shallow copy is necessary for some operations.
            • Frame.findFrame and the similar FrameSet.convert now return std::shared_ptr<FrameSet> instead of FrameSet, and return an empty pointer instead of throwing an exception if the search is unsuccessful. This makes certain common operations easier and avoids throwing a C++ exception for a routine occurrence. However, it does require annoying boilerplate if you do want to throw an exception when the search fails, so it is worth considering a convenience function or overload of the method(s) to that throws with a specified error if the search fails.
            • Add FitsChan.getCardNames to make it easy to purge PropertyList metadata that was used to create the frame set for a SkyWcs.

            I also made some small changes to lsst::afw::geom::Transform, including:

            • Making it usable as a base class, e.g. by making the destructor virtual.
            • Adding a protected constructor that takes a std::shared_ptr<FrameSet> &&.
            • Made a python function lsst.afw.geom.python.addTransformMethods that adds pure Python methods to Transform classes and subclasses. Note that the sub-namespace "python" was chosen to match the C++ namespace we use for pybind11 helpers. I also moved addTransformMapMethods into that namespace for consistency.
            • Moved much of the unit test code into a base class, so subclasses of Transform can use it.

            Finally I tweaked afw/formatters/Utils to add an ordered version of formatFitsProperties and changed a few functions to take a reference to a PropertySet instead of a shared pointer. Few, if any, of those functions should take a shared pointer, and more could easily be converted, but at some point it gets difficult due to needing to construct a LogicalLocation so I left the rest alone.

            Show
            rowen Russell Owen added a comment - - edited I made some changes to astshim as part of this, to make it easier for Transform and Wcs: Add a copy argument to FrameSet.getFrame with a default value of true . Being able to access a shallow copy is necessary for some operations. Frame.findFrame and the similar FrameSet.convert now return std::shared_ptr<FrameSet> instead of FrameSet , and return an empty pointer instead of throwing an exception if the search is unsuccessful. This makes certain common operations easier and avoids throwing a C++ exception for a routine occurrence. However, it does require annoying boilerplate if you do want to throw an exception when the search fails, so it is worth considering a convenience function or overload of the method(s) to that throws with a specified error if the search fails. Add FitsChan.getCardNames to make it easy to purge PropertyList metadata that was used to create the frame set for a SkyWcs . I also made some small changes to lsst::afw::geom::Transform , including: Making it usable as a base class, e.g. by making the destructor virtual. Adding a protected constructor that takes a std::shared_ptr<FrameSet> && . Made a python function lsst.afw.geom.python.addTransformMethods that adds pure Python methods to Transform classes and subclasses. Note that the sub-namespace "python" was chosen to match the C++ namespace we use for pybind11 helpers. I also moved addTransformMapMethods into that namespace for consistency. Moved much of the unit test code into a base class, so subclasses of Transform can use it. Finally I tweaked afw/formatters/Utils to add an ordered version of formatFitsProperties and changed a few functions to take a reference to a PropertySet instead of a shared pointer. Few, if any, of those functions should take a shared pointer, and more could easily be converted, but at some point it gets difficult due to needing to construct a LogicalLocation so I left the rest alone.
            Hide
            rowen Russell Owen added a comment - - edited

            As part of this ticket I implemented persistence of transforms and subclasses, as follows:

            • All transforms (and subclasses) support saveToFile which saves the transform to a text file. This is written in Python (though a side branch includes a C++ version, should we decide we need that).
            • A free function readTransform reads such a text file and returns the original transform (or subclass).

            In addition, SkyWcs has the following constructors:

            • One that takes FITS metadata, which is useful for reading WCS from raw images.
            • One than creates a pure TAN WCS from relevant parameters (like the existing makeWcs function(s).
            • One that takes an ast::FrameSet. This is the constructor that will be used to make WCS that are more general than those supported by wcslib and TAN-SIP. It checks that certain required frames are present.

            A future ticket will provide a way to combine a TAN WCS and a camera geometry to provide a WCS that includes optical distortion.

            Show
            rowen Russell Owen added a comment - - edited As part of this ticket I implemented persistence of transforms and subclasses, as follows: All transforms (and subclasses) support saveToFile which saves the transform to a text file. This is written in Python (though a side branch includes a C++ version, should we decide we need that). A free function readTransform reads such a text file and returns the original transform (or subclass). In addition, SkyWcs has the following constructors: One that takes FITS metadata, which is useful for reading WCS from raw images. One than creates a pure TAN WCS from relevant parameters (like the existing makeWcs function(s). One that takes an ast::FrameSet. This is the constructor that will be used to make WCS that are more general than those supported by wcslib and TAN-SIP. It checks that certain required frames are present. A future ticket will provide a way to combine a TAN WCS and a camera geometry to provide a WCS that includes optical distortion.
            Hide
            rowen Russell Owen added a comment - - edited

            Here is a summary of methods compared to the existing Wcs class:

            Persistence:

            • Transform.saveToFile(path)
            • free function readTransform(path)

            SkyWcs methods:

            • pixelToSky(): original plus vector<SpherePoint> overload
            • skyToPixel(): original plus vector<Point2D> overload
            • getCdMatrix(): unchanged
            • getSkyOrigin(): new; return CRVAL
            • getTanWcs(position): new; return a pure Tan WCS centered at the specified position.
            • getPixelScale(position): renamed from pixelScale and added the position argument
            • getPixelArea(position): renamed from pixArea and returns square radians instead of square degrees
            • getPixelOrigin(): unchanged
            • shiftedPixelOrigin(shift): renamed from shiftReferencePixel, for consistency, and returns a deep copy instead of modifying in place (because Transform is immutable). Pair of floats only; no Extent2D overload, since this functionality is rarely needed. Warning: existing usage of this method for saving to FITS needs work, because SkyWcs keeps crpix and xy0 offset as separate information.

            Deleted methods:

            • getEquinox: don't need because we force ICRS
            • getLinearTransform (not at a point): basically returns the CD matrix. Not used.
            • isSameSkySystem: don't need because we force ICRS
            • rotateImageBy90: not used
            • flipImage: only used in one place, and it can almost certainly be done some simpler way (e.g. a utility function)
            • isFlipped: used by anetBasicAstrometry and one unit test. It is simple enough to implement, but feels like clutter (unless we also implement flipImage). Consider a pure Python free function – probably in anetBasicAstrometry itself.
            • hasDistortion: This should not be needed if we start providing properly distorted WCS in calexp. I know of no simple and robust way to implement this.

            Not yet implemented:

            • linearizePixelToSky and linearizeSkyToPixel: easy but almost certainly should be methods on Transform (with different names, such as linearizeForward and linearizeInverse). note that Transform already offers getJacobian
            Show
            rowen Russell Owen added a comment - - edited Here is a summary of methods compared to the existing Wcs class: Persistence: Transform.saveToFile(path) free function readTransform(path) SkyWcs methods: pixelToSky(): original plus vector<SpherePoint> overload skyToPixel(): original plus vector<Point2D> overload getCdMatrix(): unchanged getSkyOrigin(): new; return CRVAL getTanWcs(position): new; return a pure Tan WCS centered at the specified position. getPixelScale(position): renamed from pixelScale and added the position argument getPixelArea(position): renamed from pixArea and returns square radians instead of square degrees getPixelOrigin(): unchanged shiftedPixelOrigin(shift): renamed from shiftReferencePixel, for consistency, and returns a deep copy instead of modifying in place (because Transform is immutable). Pair of floats only; no Extent2D overload, since this functionality is rarely needed. Warning: existing usage of this method for saving to FITS needs work, because SkyWcs keeps crpix and xy0 offset as separate information. Deleted methods: getEquinox: don't need because we force ICRS getLinearTransform (not at a point): basically returns the CD matrix. Not used. isSameSkySystem: don't need because we force ICRS rotateImageBy90: not used flipImage: only used in one place, and it can almost certainly be done some simpler way (e.g. a utility function) isFlipped: used by anetBasicAstrometry and one unit test. It is simple enough to implement, but feels like clutter (unless we also implement flipImage). Consider a pure Python free function – probably in anetBasicAstrometry itself. hasDistortion: This should not be needed if we start providing properly distorted WCS in calexp. I know of no simple and robust way to implement this. Not yet implemented: linearizePixelToSky and linearizeSkyToPixel: easy but almost certainly should be methods on Transform (with different names, such as linearizeForward and linearizeInverse). note that Transform already offers getJacobian
            Hide
            price Paul Price added a comment -

            I just posted a LOT of comments on the GitHub PR. Please note that the number of comments does not reflect the quality of the code (which is good) but simply the sheer size of the submission (2300+ lines added). As always with GitHub, the trivial comments are mixed up with the major objections; please let me know if the severity level of a comment is not obvious. There are some issues which I feel strongly about (e.g., we should not use AST naming conventions in afw classes), but I think everything can be resolved fairly quickly and it's possible some of my major objections can simply be dismissed as out of scope for this ticket if you promise to address them in the near future, since I know you're keen to get this merged.

            Show
            price Paul Price added a comment - I just posted a LOT of comments on the GitHub PR. Please note that the number of comments does not reflect the quality of the code (which is good) but simply the sheer size of the submission (2300+ lines added). As always with GitHub, the trivial comments are mixed up with the major objections; please let me know if the severity level of a comment is not obvious. There are some issues which I feel strongly about (e.g., we should not use AST naming conventions in afw classes), but I think everything can be resolved fairly quickly and it's possible some of my major objections can simply be dismissed as out of scope for this ticket if you promise to address them in the near future, since I know you're keen to get this merged.
            Hide
            rowen Russell Owen added a comment -

            Thank you for the very helpful review. I know it was a lot to look at.

            I think I have addressed everything you brought up (implementing almost all of it) and will merge after a Jenkins run passes.

            Show
            rowen Russell Owen added a comment - Thank you for the very helpful review. I know it was a lot to look at. I think I have addressed everything you brought up (implementing almost all of it) and will merge after a Jenkins run passes.

              People

              • Assignee:
                rowen Russell Owen
                Reporter:
                krughoff Simon Krughoff
                Reviewers:
                Paul Price
                Watchers:
                Paul Price, Russell Owen, Simon Krughoff
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: