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

Rename invert() and getInverse() to inverted()

    XMLWordPrintable

    Details

      Description

      Implement RFC-500 by renaming the invert method of geom AffineTransform, geom LinearTransform and afw GridTransform and the getInverse method of afw Transform and astshim Mapping to inverted.

      Also rename the simplify method of astshim Mapping to simplifed for the same reason. Note that this is little used outside of astshim.

        Attachments

          Issue Links

            Activity

            No builds found.
            rowen Russell Owen created issue -
            rowen Russell Owen made changes -
            Field Original Value New Value
            Epic Link DM-14447 [ 80385 ]
            rowen Russell Owen made changes -
            Link This issue is triggered by RFC-500 [ RFC-500 ]
            rowen Russell Owen made changes -
            Risk Score 0
            rowen Russell Owen made changes -
            Assignee Russell Owen [ rowen ]
            Hide
            rowen Russell Owen added a comment -

            Retain the old names for backwards compatibility in all but astshim (because our use of astshim is fairly localized) and mark the old methods as deprecated. File a ticket to remove the old names. But if time permits, do update our current code in the DM stack to use the new names. That should go quickly.

            Show
            rowen Russell Owen added a comment - Retain the old names for backwards compatibility in all but astshim (because our use of astshim is fairly localized) and mark the old methods as deprecated. File a ticket to remove the old names. But if time permits, do update our current code in the DM stack to use the new names. That should go quickly.
            swinbank John Swinbank made changes -
            Sprint AP F18-3 [ 748 ]
            Story Points 2 4
            rowen Russell Owen made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            rowen Russell Owen made changes -
            Summary Rename inverse() and getInverse() to inverted() Rename invert() and getInverse() to inverted()
            rowen Russell Owen made changes -
            Description Implement RFC-500 by renaming the {{inverse}} method of geom AffineTransform and LinearTransform and the {{getInverse}} method of afw Transform and astshim Mapping to {{inverted}}. Also rename the {{simplify}} method of astshim Mapping to {{simplifed}}. Implement RFC-500 by renaming the {{invert}} method of geom {{AffineTransform}}, geom {{LinearTransform}} and afw {{GridTransform}} and the {{getInverse}} method of afw {{Transform}} and astshim {{Mapping}} to {{inverted}}.

            Also rename the {{simplify}} method of astshim {{Mapping}} to {{simplifed}} for the same reason. Note that this is little used outside of astshim.
            Hide
            rowen Russell Owen added a comment -

            The main work is in the following packages:

            • astshim: rename getInverse to inverted and simplify to simplified.
            • afw: add inverted to Transform and GridTransform as the preferred alternative to getInverse for Transform and invert for GridTransform.
            • geom: add inverted to LinearTransform and AffineTransform as the preferred alternative to invert.
            • jointcal: rename invert to inverted and update existing usage of classes from the packages above (if there is any; by doing the rename I handled that).

            For the other packages I just modified them to use inverted instead of the older name. I think in all cases the change was optional (because astshim is used so rarely) but no guarantees.

            I did not bother with backward compatibility for astshim and jointcal because the former is only used by afw (I think) and the latter is not used by any other package.

            Show
            rowen Russell Owen added a comment - The main work is in the following packages: astshim: rename getInverse to inverted and simplify to simplified . afw: add inverted to Transform and GridTransform as the preferred alternative to getInverse for Transform and invert for GridTransform . geom: add inverted to LinearTransform and AffineTransform as the preferred alternative to invert . jointcal: rename invert to inverted and update existing usage of classes from the packages above (if there is any; by doing the rename I handled that). For the other packages I just modified them to use inverted instead of the older name. I think in all cases the change was optional (because astshim is used so rarely) but no guarantees. I did not bother with backward compatibility for astshim and jointcal because the former is only used by afw (I think) and the latter is not used by any other package.
            rowen Russell Owen made changes -
            Reviewers John Swinbank [ swinbank ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            rowen Russell Owen made changes -
            Story Points 4 2
            Hide
            swinbank John Swinbank added a comment -

            Thanks; looks fine.

            I created a PR on afw because I don't think there was one.

            There's a minor nitpick on the on the astshim PR. In addition, I wonder about this comment:

            Each new CmpMap is simplified using `simplify` before being stored in the FrameSet.

            Without digging around in astAddFrame more than I want to, it's not obvious whether that should now be “using `simplified`”, or if this is referring to some function in AST itself (but that seems to be ast_simplify, not just simplify). (To be honest, after a couple of minutes of poking about, I couldn't find where this simplification is happening at all, so I gave up...)

            I'd like to see a ticket for removal of the deprecated functions after some future release.

            Other than that, good to go.

            Show
            swinbank John Swinbank added a comment - Thanks; looks fine. I created a PR on afw because I don't think there was one. There's a minor nitpick on the on the astshim PR. In addition, I wonder about this comment: Each new CmpMap is simplified using `simplify` before being stored in the FrameSet. Without digging around in astAddFrame more than I want to, it's not obvious whether that should now be “using `simplified`”, or if this is referring to some function in AST itself (but that seems to be ast_simplify , not just simplify ). (To be honest, after a couple of minutes of poking about, I couldn't find where this simplification is happening at all, so I gave up...) I'd like to see a ticket for removal of the deprecated functions after some future release. Other than that, good to go.
            swinbank John Swinbank made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            Hide
            rowen Russell Owen added a comment -

            Thank you very much. You were right that "using simplify" should be changed to "using simplified" in astshim FrameSet.h. However, that sounds weird so I deleted the "using simplify" instead. I also updated the other comment you noted in the astshim pull request as per a note on github.

            Show
            rowen Russell Owen added a comment - Thank you very much. You were right that "using simplify" should be changed to "using simplified" in astshim FrameSet.h . However, that sounds weird so I deleted the "using simplify" instead. I also updated the other comment you noted in the astshim pull request as per a note on github.
            rowen Russell Owen made changes -
            Link This issue is triggering DM-15417 [ DM-15417 ]
            Hide
            rowen Russell Owen added a comment - - edited

            I filed DM-15417 to remove the deprecated methods after the next major release and issued community post https://community.lsst.org/t/inverted-is-now-the-standard-method-name-to-get-an-inverse-transform/3122 to describe the changes.

            Show
            rowen Russell Owen added a comment - - edited I filed DM-15417 to remove the deprecated methods after the next major release and issued community post https://community.lsst.org/t/inverted-is-now-the-standard-method-name-to-get-an-inverse-transform/3122 to describe the changes.
            rowen Russell Owen made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Reviewers:
              John Swinbank
              Watchers:
              John Swinbank, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.