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

Formally deprecate discouraged C++ afw/geom components

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw, geom
    • Labels:
    • Story Points:
      4
    • Sprint:
      AP F19-6 (November), AP S20-1 (December)
    • Team:
      Alert Production

      Description

      A number of components in geom and afw are informally documented as deprecated. They should be re-deprecated using the RFC-557 procedure. This includes warning users in the release notes, since old-style deprecation comments weren't particularly visible.

        Attachments

          Issue Links

            Activity

            Hide
            krzys Krzysztof Findeisen added a comment -

            Removing from the sprint, as it's not clear when DM-17684 will be done and we don't want @deprecated breaking people's builds.

            Show
            krzys Krzysztof Findeisen added a comment - Removing from the sprint, as it's not clear when DM-17684 will be done and we don't want @deprecated breaking people's builds.
            Hide
            krzys Krzysztof Findeisen added a comment -

            There may be some duplication of effort between this issue and DM-18544, as John Parejko noted that the latter will need to make some improvements to the current policy for deprecating pybind11 wrappers.

            Show
            krzys Krzysztof Findeisen added a comment - There may be some duplication of effort between this issue and DM-18544 , as John Parejko noted that the latter will need to make some improvements to the current policy for deprecating pybind11 wrappers.
            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            As discussed on #dm, we should deprecate the use of lsst.afw.geom names for types that are now in lsst.geom, but it's not obvious how to do so.

            Show
            krzys Krzysztof Findeisen added a comment - - edited As discussed on #dm , we should deprecate the use of lsst.afw.geom names for types that are now in lsst.geom , but it's not obvious how to do so.
            Hide
            tjenness Tim Jenness added a comment -

            There are also 4 methods in utils package that have been deprecated for years.

            Show
            tjenness Tim Jenness added a comment - There are also 4 methods in utils package that have been deprecated for years.
            Hide
            ktl Kian-Tat Lim added a comment -

            If there's no actual usage of lsst.afw.geom names in our code, then I think deprecation in the release notes and via Community posting is sufficient.  I've tried out various mechanisms for issuing deprecation warnings, but they're either too complicated or have undesirable side effects or both.

            Show
            ktl Kian-Tat Lim added a comment - If there's no actual usage of lsst.afw.geom names in our code, then I think deprecation in the release notes and via Community posting is sufficient.  I've tried out various mechanisms for issuing deprecation warnings, but they're either too complicated or have undesirable side effects or both.
            Hide
            tjenness Tim Jenness added a comment -

            I'm pretty sure there is still code that needs to be updated to use lsst.geom.

            Show
            tjenness Tim Jenness added a comment - I'm pretty sure there is still code that needs to be updated to use lsst.geom.
            Hide
            tjenness Tim Jenness added a comment -

            Regarding afwGeom, wouldn't it be easier to announce the formal deprecation of the aliases and then in six months time remove all the code? Rather than trying to sort out how to get the deprecations to turn up in the right place? The alternative is to implement afwGeom as an explicit trampoline rather than the current alias.

            John Swinbank, Gabriele Comoretto [X], did we ever announce the removal of the afwGeom code and move to geom?

            Show
            tjenness Tim Jenness added a comment - Regarding afwGeom, wouldn't it be easier to announce the formal deprecation of the aliases and then in six months time remove all the code? Rather than trying to sort out how to get the deprecations to turn up in the right place? The alternative is to implement afwGeom as an explicit trampoline rather than the current alias. John Swinbank , Gabriele Comoretto [X] , did we ever announce the removal of the afwGeom code and move to geom?
            Hide
            tjenness Tim Jenness added a comment - - edited

            For the record we announced the removal of these geom functions from afw in v16.0 and we stated that aliases would be provided but that they should not be used in new code. John Swinbank, Kian-Tat Lim, Should we add a formal deprecation to the v18.0 release notes?

            https://pipelines.lsst.io/releases/notes.html#reworked-geom-package-replacing-much-of-afw-geom

            Show
            tjenness Tim Jenness added a comment - - edited For the record we announced the removal of these geom functions from afw in v16.0 and we stated that aliases would be provided but that they should not be used in new code. John Swinbank , Kian-Tat Lim , Should we add a formal deprecation to the v18.0 release notes? https://pipelines.lsst.io/releases/notes.html#reworked-geom-package-replacing-much-of-afw-geom
            Hide
            swinbank John Swinbank added a comment -

            I don't understand the discussion above. It sounds like this ticket was blocked on DM-17684, which is now done — that presumably supersedes all discussion about “how to get the deprecations to turn up in the right place”. Can't we just work this ticket as originally intended?

            (afw)Geom changes were announced at https://pipelines.lsst.io/v/v18_0_0/releases/notes.html#release-v16-0-new-geom. That predates current conventions for announcing deprecations.

            Show
            swinbank John Swinbank added a comment - I don't understand the discussion above. It sounds like this ticket was blocked on DM-17684 , which is now done — that presumably supersedes all discussion about “how to get the deprecations to turn up in the right place”. Can't we just work this ticket as originally intended? (afw)Geom changes were announced at https://pipelines.lsst.io/v/v18_0_0/releases/notes.html#release-v16-0-new-geom . That predates current conventions for announcing deprecations.
            Hide
            swinbank John Swinbank added a comment -

            Should we add a formal deprecation to the v18.0 release notes?

            Sounds like a good idea.

            Show
            swinbank John Swinbank added a comment - Should we add a formal deprecation to the v18.0 release notes? Sounds like a good idea.
            Hide
            tjenness Tim Jenness added a comment -

            John Swinbank's comment saying "Can't we just do this" resulted in my brain getting a kick. It seems that we can use the new deprecate_pybind11 function to add the deprecation warnings for the half dozen geom functions that we are importing. It's much easier than I realized.

            from lsst.utils import deprecate_pybind11
             
            # for backwards compatibility make lsst.geom public symbols available in lsst.afw.geom
            from lsst.geom import *
             
            Angle = deprecate_pybind11(Angle, reason="Blah", category=FutureWarning)
            del deprecate_pybind11
            

            seems to work fine so I'll add the warnings.

            Show
            tjenness Tim Jenness added a comment - John Swinbank 's comment saying "Can't we just do this" resulted in my brain getting a kick. It seems that we can use the new deprecate_pybind11 function to add the deprecation warnings for the half dozen geom functions that we are importing. It's much easier than I realized. from lsst.utils import deprecate_pybind11   # for backwards compatibility make lsst.geom public symbols available in lsst.afw.geom from lsst.geom import *   Angle = deprecate_pybind11(Angle, reason = "Blah" , category = FutureWarning) del deprecate_pybind11 seems to work fine so I'll add the warnings.
            Hide
            tjenness Tim Jenness added a comment -

            I did end up deprecating the afwGeom forwarding routines in DM-20546. I couldn't work out a way to get the units such as degrees and radians to deprecate properly since they are constants and not classes.

            I did not sort out any of the C++ deprecated code in afw or C++ deprecations in geom (I only see one in LinearTransform) so I'm leaving this ticket open for that work.

            Show
            tjenness Tim Jenness added a comment - I did end up deprecating the afwGeom forwarding routines in DM-20546 . I couldn't work out a way to get the units such as degrees and radians to deprecate properly since they are constants and not classes. I did not sort out any of the C++ deprecated code in afw or C++ deprecations in geom (I only see one in LinearTransform) so I'm leaving this ticket open for that work.
            Hide
            tjenness Tim Jenness added a comment -

            I learned yesterday that afwImage.readMetadata should be deprecated and afwFits.readMetadata should be used instead.

            Show
            tjenness Tim Jenness added a comment - I learned yesterday that afwImage.readMetadata should be deprecated and afwFits.readMetadata should be used instead.
            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            A question for an afw.table expert in the house: a comment on ArrayKey says that Key<Array<T>> is deprecated in favor of ArrayKey<T>. However, afw.table itself extensively uses Key<Array<T>>, including in generic code (e.g., the templated wrappers for Schema). It looks like transitioning to ArrayKey would be a lot of nontrivial code changes.

            Is this deprecation something that got started, but then abandoned, or should there be a separate ticket for handling this case? I certainly don't want it to slow down removal of the other old code we have in afw.

            Show
            krzys Krzysztof Findeisen added a comment - - edited A question for an afw.table expert in the house: a comment on ArrayKey says that Key<Array<T>> is deprecated in favor of ArrayKey<T> . However, afw.table itself extensively uses Key<Array<T>> , including in generic code (e.g., the templated wrappers for Schema ). It looks like transitioning to ArrayKey would be a lot of nontrivial code changes. Is this deprecation something that got started, but then abandoned, or should there be a separate ticket for handling this case? I certainly don't want it to slow down removal of the other old code we have in afw .
            Hide
            krzys Krzysztof Findeisen added a comment -

            I discussed the ArrayKey question with Jim Bosch on Slack; resolution is to reword the comment to say that ArrayKey is "preferred" but to make no mention of deprecation.

            Show
            krzys Krzysztof Findeisen added a comment - I discussed the ArrayKey question with Jim Bosch on Slack; resolution is to reword the comment to say that ArrayKey is "preferred" but to make no mention of deprecation.
            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            Hi Tim Jenness, would you be willing to review this issue? It's about 590 lines, including removing afw's use of its own deprecated code. To keep the review sane, I did not do any incidental code cleanup (copyright, refactoring, etc.), not even on separate commits.

            One commit, lsst/afw@488744f, adds two pybind11 deprecations that conditionally call warnings.warn for specific overloads instead of unconditionally using deprecate_pybind11 (the false positive rate in the latter case would have been very high). This breaks a "must" style guide rule, so could Kian-Tat Lim please approve those changes? Both cases are in backgroundContinued.py.

            Show
            krzys Krzysztof Findeisen added a comment - - edited Hi Tim Jenness , would you be willing to review this issue? It's about 590 lines, including removing afw 's use of its own deprecated code. To keep the review sane, I did not do any incidental code cleanup (copyright, refactoring, etc.), not even on separate commits. One commit, lsst/afw@488744f , adds two pybind11 deprecations that conditionally call warnings.warn for specific overloads instead of unconditionally using deprecate_pybind11 (the false positive rate in the latter case would have been very high). This breaks a "must" style guide rule , so could Kian-Tat Lim please approve those changes? Both cases are in backgroundContinued.py .
            Hide
            tjenness Tim Jenness added a comment -

            Looks good to me. I'll leave in review to let Kian-Tat Lim comment.

            Show
            tjenness Tim Jenness added a comment - Looks good to me. I'll leave in review to let Kian-Tat Lim comment.
            Hide
            ktl Kian-Tat Lim added a comment -

            As said on the PR: This looks fine. In these particular cases of using warnings.warn, I approve because an appropriate Python location for the code already exists, so the hand-wrapping is not too arduous nor difficult to understand.

            Show
            ktl Kian-Tat Lim added a comment - As said on the PR: This looks fine. In these particular cases of using warnings.warn, I approve because an appropriate Python location for the code already exists, so the hand-wrapping is not too arduous nor difficult to understand.

              People

              Assignee:
              krzys Krzysztof Findeisen
              Reporter:
              krzys Krzysztof Findeisen
              Reviewers:
              Kian-Tat Lim, Tim Jenness
              Watchers:
              Gabriele Comoretto [X] (Inactive), John Parejko, John Swinbank, Kian-Tat Lim, Krzysztof Findeisen, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.