Details
-
Type:
Improvement
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: astshim
-
Labels:
-
Story Points:1
-
Epic Link:
-
Sprint:Alert Production S17 - 3
-
Team:Alert Production
Description
PolyTran's constructor that takes only forward coefficients provides an iterative inverse by default (as does its other constructor if the forward coefficient array is empty). This seems like a bad idea for several reasons:
- Most polynomials do not have unique inverses. That said, the iterative inverse will do sensible things in that situation (nan if no value exists, else a valid value if multiple choices exist).
- The iterative inverse is much less efficient to compute than the fit inverse provided by `PolyMap.polyTran`.
I propose to disable the iterative inverse by default, using the philosophy of "when in doubt, refuse to guess".
Attachments
Activity
Are you saying there isn't an option to let AST calculate the inverse itself? The iterative inverse for PolyMaps was added specifically to support SIP FITS images and the like which usually lacked a defined inverse in the FITS headers and people complained when they couldn't convert from sky back to pixel coordinates.
I am saying that the user should ask for the iterative inverse. It's trivial to do. Just specify "IterInverse=1" when constructing the PolyMap. Please see the docs for the 2nd constructor of PolyMap.
As I understand it, AST does define an iterative inverse by default (the documentation for astPolyMap claiming the inverse is undefined if ncoeff_i == 0 is an error), so arguably astshim should have analogous behavior.
For users not familiar with AST, however, I agree that providing an approximate inverse only on request is less surprising.
Krzysztof Findeisen
added a comment - - edited As I understand it, AST does define an iterative inverse by default (the documentation for astPolyMap claiming the inverse is undefined if ncoeff_i == 0 is an error), so arguably astshim should have analogous behavior.
For users not familiar with AST, however, I agree that providing an approximate inverse only on request is less surprising. Looks good, I have a few suggestions for documentation changes.
Krzysztof Findeisen
added a comment - Looks good, I have a few suggestions for documentation changes.
Implemented. I also greatly expanded the explanation of how to obtain an inverse in the class documentation for PolyMap and expanded the associated unit test to test inverses for a polynomial whose inverse is not single-valued.