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

Un-deprecate APIs for afw.math.Background and afw.math.BackgroundList

    Details

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

      Description

      In 2012, several changes were introduced (commits lsst/afw@fd4b25a, b14beb7 and 883e411) to the lsst.afw.math.Background API to try to make it less stateful and more based on explicit arguments. The following components (C++, unless otherwise stated) were noted as deprecated in the API documentation:

      • BackgroundControl constructors that take Interpolate::Style or a style string
      • The zero-argument form of Background.getImage (getImageF in Python)
      • BackgroundMI.getPixel (all overloads)
      • use of Python's BackgroundList with self-contained Background objects instead of tuples of Background and configuration information

      Over the following seven years, little attempt was made to actually migrate code that used Background from the deprecated API to the new one. In DM-17566, I applied our then-new deprecation policy to these and other afw components, causing afw to emit warnings when the old APIs are used. Per policy, the APIs are currently slated for removal in Science Pipelines release 21 (DM-22814).

      While migration was straightforward for most of the components affected by DM-17566, the changes for Background proved very hard to implement. Among other problems:

      • Much of the old code relied on default values for style parameters in BackgroundControl. Replacing these defaults with explicit arguments to Background.getImage can be difficult, especially when getImage is called in a context far removed from the original background configuration. (DM-17566, DM-23113)
      • The code's current hybrid state makes it very easy to introduce both human error and consistency bugs, since different (and sometimes multiple) classes are responsible for each aspect of Background configuration. For example, interpolation style is now handled differently from undersampling style. (DM-23113)
      • The new BackgroundList API is very difficult to use correctly. (DM-23056)
      • Changes to where interpolation style is handled cascade to other APIs (because they start or stop being responsible for styles). Many of these were not originally marked as deprecated. (DM-22814, DM-23076)

      I propose that we take these APIs out of deprecated status, on the grounds that the consequences of the migration were not fully appreciated and planned for when it was first proposed. This will not solve all our problems with Background, of course, but I think the previous status quo is better than the current situation.

      The specific changes needed to implement this RFC are:

      I suggest we don't try to revert changes to bring code into compliance with the "new" APIs; since these APIs existed prior to DM-17566, they will still work.

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment -

            I support this RFC, and fully agree with Krzysztof Findeisen's assessment. Trying to remove usage of the APIs that were deprecated back in 2012 has made our code more fragile and confusing than it was before.

            Show
            swinbank John Swinbank added a comment - I support this RFC, and fully agree with Krzysztof Findeisen 's assessment. Trying to remove usage of the APIs that were deprecated back in 2012 has made our code more fragile and confusing than it was before.
            Hide
            krzys Krzysztof Findeisen added a comment -

            In the absence of any objections, I'm adopting as proposed.

            Show
            krzys Krzysztof Findeisen added a comment - In the absence of any objections, I'm adopting as proposed.

              People

              • Assignee:
                krzys Krzysztof Findeisen
                Reporter:
                krzys Krzysztof Findeisen
                Watchers:
                John Swinbank, Krzysztof Findeisen, Robert Lupton, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Planned End:

                  Summary Panel