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

Create TransformBoundedField

    Details

      Description

      Jointcal's improved photometric fit is done on the full focal plane, and thus we need its persisted output to be able to convert pixel coordinates on one CCD to the focal plane. Probably the easiest way to implement this is to create an astBoundedField that we can put into a PhotoCalib.

      We may eventually want to replace PhotoCalib's use of BoundedField internally with AST, but for consistency with Jim Bosch's meas_mosaic work, keeping the PhotoCalib interface the same is probably best.

      For reference, jointcal's photometric output looks something like f(g(x,y))*h(x,y), where (x,y) are in pixel coordinates, g is the pixel to focal plane transformation, f is a Chebyshev polynomial, and h is some chip-level transform (currently a constant).

      Tagging this PairCoding, as it might be nice for me to sit with Russell Owen as he works on it, to aid my familiarity with AST.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment - - edited

            The work consists of the following:

            • Add TransformBoundedField to afw math, including a pybind11 wrapper and unit test.
            • Add ast::Object::operator== and operator!=. This was wanted to support TransformBoundedField::operator==. It's picky in that all attributes must be identical (though it does ignore the difference between an attribute that is not set and one that is set to the default value).
            • Add a showComments argument to ast::Object::show to simplify the implementation of operator==. It is debatable if that is an optimal addition; another option is to have operator== itself create the string representation. I think it's pragmatic but feel free to push back.

            Known limitations:

            • TransformBoundedField::mean and the closely related integrate method are not yet implemented. This will require some care. Deferred to DM-11211
            • The vector version of TransformBoundedField::evaluate copies the input data because there is no way to combine two 1-dimensional ndarrays into one 2-dimensional ndarray. It is worth considering adding a variadic version of ast::Mapping::applyForward and applyInverse that takes a list of 1-dimensional arrays (AST itself already supports that).
            Show
            rowen Russell Owen added a comment - - edited The work consists of the following: Add TransformBoundedField to afw math, including a pybind11 wrapper and unit test. Add ast::Object::operator== and operator!= . This was wanted to support TransformBoundedField::operator== . It's picky in that all attributes must be identical (though it does ignore the difference between an attribute that is not set and one that is set to the default value). Add a showComments argument to ast::Object::show to simplify the implementation of operator== . It is debatable if that is an optimal addition; another option is to have operator== itself create the string representation. I think it's pragmatic but feel free to push back. Known limitations: TransformBoundedField::mean and the closely related integrate method are not yet implemented. This will require some care. Deferred to DM-11211 The vector version of TransformBoundedField::evaluate copies the input data because there is no way to combine two 1-dimensional ndarrays into one 2-dimensional ndarray. It is worth considering adding a variadic version of ast::Mapping::applyForward and applyInverse that takes a list of 1-dimensional arrays (AST itself already supports that).
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Minor comments on PR, otherwise ok. My main gripe is the equality comparison through comparison of string representations.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Minor comments on PR, otherwise ok. My main gripe is the equality comparison through comparison of string representations.
            Hide
            rowen Russell Owen added a comment - - edited

            I use the string representation to test equality in AST because AST offers no other solution at this time. I did update it to pay attention to unset vs. set parameters for fear it might make a difference in some subtle way (e.g. finding frames in frame sets). That makes it easier to understand and simplifies the code.

            For the record: I use a variable-length string to persist Transforms because that seemed the best option for persisting the contained AST FrameSet. AST does support two alternatives: AST-specific FITS header cards and XML. Neither seemed better to my mind.

            I simplified the way TransformBoundedField is persisted, so that the AST dependency is basically hidden.

            In order to do this I added Transform persistence in the form of readStream, readString, writeStream, and writeString methods. I also added those methods to SkyWcs. To share code, most of the work is done by two new utility functions in afw::geom::detail.

            Please have another look at afw since the changes are significant. I also applied your suggested changes to astshim, but those were small, so feel free to check astshim or not, as you prefer.

            Show
            rowen Russell Owen added a comment - - edited I use the string representation to test equality in AST because AST offers no other solution at this time. I did update it to pay attention to unset vs. set parameters for fear it might make a difference in some subtle way (e.g. finding frames in frame sets). That makes it easier to understand and simplifies the code. For the record: I use a variable-length string to persist Transforms because that seemed the best option for persisting the contained AST FrameSet. AST does support two alternatives: AST-specific FITS header cards and XML. Neither seemed better to my mind. I simplified the way TransformBoundedField is persisted, so that the AST dependency is basically hidden. In order to do this I added Transform persistence in the form of readStream , readString , writeStream , and writeString methods. I also added those methods to SkyWcs . To share code, most of the work is done by two new utility functions in afw::geom::detail . Please have another look at afw since the changes are significant. I also applied your suggested changes to astshim, but those were small, so feel free to check astshim or not, as you prefer.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment - - edited

            Russell Owen thanks for the changes. Looks good to me. The only request I have is to add a constexpr for the version magic number.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - - edited Russell Owen thanks for the changes. Looks good to me. The only request I have is to add a constexpr for the version magic number.
            Hide
            rowen Russell Owen added a comment -

            Thank you for the re-review. I added the version magic number as you requested, fixed the missing override, and fixed up the copyright dates for various relevant files. I'll merge after a Jenkins run passes.

            Show
            rowen Russell Owen added a comment - Thank you for the re-review. I added the version magic number as you requested, fixed the missing override , and fixed up the copyright dates for various relevant files. I'll merge after a Jenkins run passes.

              People

              • Assignee:
                rowen Russell Owen
                Reporter:
                Parejkoj John Parejko
                Reviewers:
                Pim Schellart [X] (Inactive)
                Watchers:
                Jim Bosch, John Parejko, John Swinbank, Krzysztof Findeisen, Pim Schellart [X] (Inactive), Russell Owen, Simon Krughoff
              • Votes:
                0 Vote for this issue
                Watchers:
                7 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel