Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-58

Refactor afw.math.Approximate, .BackgroundMI, .GaussianProcess to share interface

    Details

    • Type: RFC
    • Status: Implemented
    • Resolution: Done
    • Component/s: DM
    • Labels:
      None
    • Location:
      comments

      Description

      The LSST Stack currently models 2D surfaces in the background-matching and background-estimation components. In the near future, code that uses 2D modeled surfaces will grow to include aperture corrections and PSF/zero-point interpolation over a chip or focal plane. We have three classes which can model/interpolate 2D surfaces:

      • lsst.afw.math.Approximate (Chebyshev polynomial fit)
      • lsst.afw.math.BackgroundMI (Spline interpolation - CONSTANT, LINEAR, NATURAL_SPLINE, CUBIC_SPLINE, CUBIC_SPLINE_PERIODIC, AKIMA_SPLINE, AKIMA_SPLINE_PERIODIC)
      • lsst.afw.math.GaussianProcess (Gaussian process interpolation)

      This current state of affairs is lacking in that:

      • The APIs for the three classes differ, which makes it difficult to swap one object in for the other. We end up with repetitive control structures the client code. For example, MatchBackgroundsTask returns either a lsst.afw.Approximate object or a lsst.afw.BackgroundMI object depending on whether it is configured to use polynomials or splines. The result is bad code that looks like:

         backgroundImage = backgroundModel.getImage() if \
                            self.matchBackgrounds.config.usePolynomial else \
                            backgroundModel.getImageF()

      • No persistence for Gaussian Process and Approximate.

      Whereas the interpolated/fitted 2D models differ in their internal representation (e.g. coefficients vs. points to interpolate between, fitting algorithms and evaluation algorithms) they are similar in the following ways. They are have an XY domain. They are created from points (either gridded/image or non-gridded/scattered). Client code uses them primarily to evaluate at (x, y) and getImage().

      Users should be able to easily swap one class for another, and storing the 2D models is necessary for aperture corrections. These requirements call for an abstract base class that contains an interface for accessing the interpolated/approximated function while making no assumptions about its functional form. Jim Bosch implemented this (called BoundedField) on the HSC side with support for gridded and non-gridded/scattered Chebyshev polynomials. https://github.com/HyperSuprime-Cam/afw/compare/releases/S14A_0...tickets/DM-796 has been pulled over to the LSST side unchanged in afw: u/yusra/DM-740.

      As I adapt BoundedField for the LSST stack (DM-1191), I'm requesting comments on the API in the following four categories:

      1) Name for abstract base class
      Proposal: Model2D (with subclasses named ChebyshevModel2D, GaussianProcessModel2D, etc...)
      The name should convey that it is:

      • Two-dimensional: "Surface", "2D", "Field"
      • Has an XY Domain: "Bounded"
      • It is an inexact representation: "Model"

      2) Proposed API for creating/building a Model2D.

      • Assumption: Users will not be writing their own Model2D classes. => A simple static factory (as opposed to a dynamic plug-in system such as the measurement framework) should be sufficient. Like our current Background class's interp_style ENUM, we would maintain an ENUM of available subclasses of Model2D.
      • Example usage - where "fit" is the name of our subclassed "create (from data)" method:

        ctrl = lsst.afw.math.Model2DControl('CHEBYSHEV', configStruct)
        chebyshevModel2D = lsst.afw.math.Model2D.fit(x, y, z, ctrl)
         
        ctrl = lsst.afw.math.Model2DControl('AKIMA_SPLINE')
        interpModel2D = lsst.afw.math.Model2D.fit(x, y, z, ctrl)
        interpModel2D.getImage()	
        

        Note: The current Background API specifies the interpolation type in getImage() not BackgroundControl since the work is done in the evaluation stage instead of the creation stage. This distinction is at the crux of our original Approximate vs. Interpolate dichotomy, but is an implementation detail. All client code can use both Approximate/Interpolate interchangeably. In the the proposed API, this could be handled: ctrl = lsst.afw.math.Model2DControl('INTERPOLATE') where the "INTERPOLATE" enum would signal that an an interpolationModel2D be instantiated, which would take an extra argument in getImage('afwMath.Interpolate.AKIMA_SPLINE'). It would be cleaner, however, to require the interpolation style in the object creation stage, even though its not used until evaluate() or getImage(), since we gain a lot by having an identical interface for using Model2D objects. This of course begs for efficient methods to turning one type of Model2D into another.

      3) Operating on a Model2D We'd like to do basic operations on these objects, such as adding and multiplying by scalars and performing transformations on the x/y domain. Jim Bosch recommended Model2D objects remain immutable (original HSC BoundedFields are). Operations would create new objects, rather than alter them. For example:

      • chebyshevModel2D = 2 * halfchebyshevModel2D
      • akimaModel2D = lsst.afw.math.Model2D.fit(linearInterpModel2D, ctrl) #see note in section 3

      4) using a Model2D:

      • model2D.evaluate(x,y)
      • model2D.addToImage(image); model2D.multiplyImage(image)
      • model2D.fillImage(image)
      • model2D.getImage() Note: This requires that a Model2D be aware of its bounding box. Currently Approximate and Background have getImage() methods. When adapting the client code, I will also do a cost/benefit analysis of maintaining this.

      Epic link: DM-1991
      Implementation Issue: DM-740
      Some initial thoughts can be found on the Requirements page: https://confluence.lsstcorp.org/pages/viewpage.action?pageId=33161587

      This RFC will be open until EOB Friday, I will take comments into account for the final design.

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment -

            I'm sure what Yusra has told you is correct.

            Show
            swinbank John Swinbank added a comment - I'm sure what Yusra has told you is correct.
            Hide
            tjenness Tim Jenness added a comment -

            Ok, Yusra AlSayyad please add the missing tickets.

            Show
            tjenness Tim Jenness added a comment - Ok, Yusra AlSayyad please add the missing tickets.
            Hide
            yusra Yusra AlSayyad added a comment -

            I added "is triggered by RFC-58" links to the unimplemented tickets. They should show up in the link list above now.

            Show
            yusra Yusra AlSayyad added a comment - I added "is triggered by RFC-58 " links to the unimplemented tickets. They should show up in the link list above now.
            Hide
            swinbank John Swinbank added a comment -

            Retiring, per discussion on DM-740.

            Show
            swinbank John Swinbank added a comment - Retiring, per discussion on DM-740 .
            Hide
            swinbank John Swinbank added a comment -

            Ugh! Except I hit the wrong button, and I believe there is no workflow to “un-implement” an RFC. Sorry for the confusion; please regard this RFC as withdrawn, not implemented.

            Show
            swinbank John Swinbank added a comment - Ugh! Except I hit the wrong button, and I believe there is no workflow to “un-implement” an RFC. Sorry for the confusion; please regard this RFC as withdrawn , not implemented.

              People

              • Assignee:
                yusra Yusra AlSayyad
                Reporter:
                yusra Yusra AlSayyad
                Watchers:
                Jim Bosch, John Swinbank, Kian-Tat Lim, Robert Lupton, Russell Owen, Simon Krughoff, Tim Jenness, Yusra AlSayyad
              • Votes:
                0 Vote for this issue
                Watchers:
                8 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Planned End:

                  Summary Panel