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

PolyTran should not provide an iterative inverse by default

    XMLWordPrintable

Details

    • Improvement
    • Status: Done
    • Resolution: Done
    • None
    • astshim
    • 1
    • Alert Production S17 - 3
    • 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

          rowen Russell Owen added a comment - - edited

          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.

          rowen Russell Owen added a comment - - edited 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.
          tjenness Tim Jenness added a comment -

          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.

          tjenness Tim Jenness added a comment - 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.

          rowen Russell Owen added a comment - 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 .
          krzys 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.

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

          krzys Krzysztof Findeisen added a comment - Looks good, I have a few suggestions for documentation changes.

          People

            rowen Russell Owen
            rowen Russell Owen
            Krzysztof Findeisen
            Krzysztof Findeisen, Russell Owen, Tim Jenness
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Jenkins

                No builds found.