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

Replace XYTransform::linearizeTransform

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Story Points:
      2
    • Sprint:
      Alert Production S17 - 6
    • Team:
      Alert Production

      Description

      Many clients of XYTransform use its methods linearizeForwardTransform and linearizeReverseTransform. We do not yet have analogous code for Transform. This functionality can be implemented as either a method on Transform or a separate function; the latter would provide better encapsulation.

        Attachments

          Issue Links

            Activity

            Hide
            krzys Krzysztof Findeisen added a comment -

            Hi Russell Owen, please review this ticket. You don't need to (but of course can) review the RFC-215/RFC-229 commits that come before the commit that implements linearizeTransform.

            Show
            krzys Krzysztof Findeisen added a comment - Hi Russell Owen , please review this ticket. You don't need to (but of course can) review the RFC-215 / RFC-229 commits that come before the commit that implements linearizeTransform .
            Hide
            rowen Russell Owen added a comment -

            SkyWcs presently has getTanWcs, which arguably does the same thing, but only for WCS. I wonder if that should go away and be replaced by this function?

            Also, I confess I lean towards making this a method (e.g. getLinearized(point)) of `Transform`, if only because the templating disappears. But your choice also seems fine and you wrote it so you get to decide. But maybe the above question speaks to this one way or the other.

            This looks excellent. I found one possible bug in a unit test, but otherwise good to go.

            Show
            rowen Russell Owen added a comment - SkyWcs presently has getTanWcs , which arguably does the same thing, but only for WCS. I wonder if that should go away and be replaced by this function? Also, I confess I lean towards making this a method (e.g. getLinearized(point) ) of `Transform`, if only because the templating disappears. But your choice also seems fine and you wrote it so you get to decide. But maybe the above question speaks to this one way or the other. This looks excellent. I found one possible bug in a unit test, but otherwise good to go.
            Hide
            rowen Russell Owen added a comment -

            Krzysztof Findeisen and I discussed the two issues I raised and concluded that it is unclear what getLinearized it return for a transform from or to SpherePoint; there are two choices:

            • A line segment in X,Y maps to an arc of a great circle in spherical coordinates (like TAN WCS)
            • A line segment in X,Y maps to a delta RA, delta Dec
              The latter is what the function did, but we feared users might prefer the former, or want a choice. So we decided the best course is to only define getLinearized for Cartesian transforms (those where neither end is spherical) for now.

            Thus getLinearized should stay a free function, because that makes it easy to define for a subset of template parameters.

            (Also we will leave getTanWcs as a method on SkyWcs because it is convenient and there is no strong reason to change it to a free function.)

            Show
            rowen Russell Owen added a comment - Krzysztof Findeisen and I discussed the two issues I raised and concluded that it is unclear what getLinearized it return for a transform from or to SpherePoint ; there are two choices: A line segment in X,Y maps to an arc of a great circle in spherical coordinates (like TAN WCS) A line segment in X,Y maps to a delta RA, delta Dec The latter is what the function did, but we feared users might prefer the former, or want a choice. So we decided the best course is to only define getLinearized for Cartesian transforms (those where neither end is spherical) for now. Thus getLinearized should stay a free function, because that makes it easy to define for a subset of template parameters. (Also we will leave getTanWcs as a method on SkyWcs because it is convenient and there is no strong reason to change it to a free function.)
            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            Oops, major design oversight: XYTransform::linearizeTransform returns an AffineTransform, which is not an XYTransform, so its successor ought not to return a Transform. Will fix on DM-5922.

            (I checked, and some high-level packages do require an AffineTransform).

            Show
            krzys Krzysztof Findeisen added a comment - - edited Oops, major design oversight: XYTransform::linearizeTransform returns an AffineTransform , which is not an XYTransform , so its successor ought not to return a Transform . Will fix on DM-5922 . (I checked, and some high-level packages do require an AffineTransform ).

              People

              • Assignee:
                krzys Krzysztof Findeisen
                Reporter:
                krzys Krzysztof Findeisen
                Reviewers:
                Russell Owen
                Watchers:
                Krzysztof Findeisen, Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel