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
    • Epic Link:
    • 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

          blocks

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-20564 Do release 20.0.0 of science pipelines

          • Done
          is blocked by

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-17684 Enable use of "deprecated" package in new conda environment

          • Done

          RFC - An issue whose primary purpose is to advertise an intent and allow stakeholders to comment. RFC-557 Deprecation procedure for methods and classes

          • Implemented
          is triggering

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-22814 Remove afw APIs deprecated in DM-17566

          • Done
          relates to

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-20546 Cleanup some afw deprecations

          • Done

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-18544 Add python-level deprecation warnings for Calib interface

          • Done

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-22276 Remove previously deprecated afw components

          • Invalid

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-23113 afwMath.Background.getImageF() unexpectedly ignores controller's undersample setting

          • Invalid

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-20566 Remove aliased afwGeom geom usage from stack

          • Done

          RFC - An issue whose primary purpose is to advertise an intent and allow stakeholders to comment. RFC-687 Un-deprecate APIs for afw.math.Background and afw.math.BackgroundList

          • Implemented

            Activity

            Ascending order - Click to sort in descending order
            13 older comments
            Hide
            Permalink
            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
            Permalink
            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
            Permalink
            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
            Permalink
            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
            Permalink
            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.