# Replace XYTransform::linearizeTransform

XMLWordPrintable

## Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Story Points:
2
• Sprint:
• Team:

## 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.

## Activity

Hide
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
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
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
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
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
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
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
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:
Krzysztof Findeisen
Reporter:
Krzysztof Findeisen
Reviewers:
Russell Owen
Watchers:
Krzysztof Findeisen, Russell Owen