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

PolyMap.polyTran does not clear IterInverse

    XMLWordPrintable

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: astshim

      Description

      PolyMap.polyTran replaces the forward or reverse coefficients with a fit. But if it is used to fit the inverse (as is typical) and the original PolyMap has an iterative inverse, the returned mapping will use that iterative inverse and ignore the fit coefficients.

      I have asked David Berry if astPolyTran is behaving as it should by copying the value of IterInverse instead of clearing it. I am guessing it is intentional, in which case I can see three solutions:

      • Have astshim clear IterInverse in the returned mapping when appropriate.
      • Document the problem and allow PolyMap to be modified by setting IterInverse and, presumably, the two related properties, thus breaking the rule that mappings are immutable (other than Id and Ident). Allowing these properties to be set might be useful for tweaking behavior.
      • Document the problem and be done with it. I feel this is too surprising to consider.

      Of these I feel the first, clearing IterInverse when appropriate, is most appropriate and least surprising. However, it will require a bit of care, as the original mapping may be inverted and polyTran can fit either direction. I believe the logic is as follows:

      If the mapping being fit has IterInverse set:
      If the mapping being fit is not inverted and polyTran is fitting the inverse:
      unset IterInverse in the returned mapping
      if the mapping being fit is inverted and polyTran is fitting the forward direction:
      unset IterInverse in the returned mapping

      Which simplifies to:
      If polyMap IterInverse is set and polyTran forward == PolyMap.isInverted():
      unset IterInverse in the returned mapping

      Independently it is worth considering allowing modifying the iterative inverse parameters but I'd like a clearer need before doing so.

        Attachments

          Activity

          Hide
          rowen Russell Owen added a comment - - edited

          David Berry modified AST to have astPolyTran clear the IterInverse flag.

          Unfortunately he added a bug: he clears the flag if that direction is not the one being fit by PolyMap.polyTran, meaning the user is fitting the opposite to an iterative function, which is not a very reasonable thing to do anyway. I have chosen to raise an exception in that case.

          Show
          rowen Russell Owen added a comment - - edited David Berry modified AST to have astPolyTran clear the IterInverse flag. Unfortunately he added a bug: he clears the flag if that direction is not the one being fit by PolyMap.polyTran, meaning the user is fitting the opposite to an iterative function, which is not a very reasonable thing to do anyway. I have chosen to raise an exception in that case.
          Hide
          rowen Russell Owen added a comment - - edited

          Do you have time for this? It's a fairly small change to astshim. There is an associated update to starlink_ast, but we are just adopting the code directly from David Berry so I don't think you need to review that. Nonetheless I will make a starlink_ast pull request in case you want to see what changed.

          Show
          rowen Russell Owen added a comment - - edited Do you have time for this? It's a fairly small change to astshim. There is an associated update to starlink_ast, but we are just adopting the code directly from David Berry so I don't think you need to review that. Nonetheless I will make a starlink_ast pull request in case you want to see what changed.
          Hide
          nlust Nate Lust added a comment -

          A few minor things, but looks good

          Show
          nlust Nate Lust added a comment - A few minor things, but looks good
          Hide
          rowen Russell Owen added a comment -

          I didn't see any requested changes on github (or here) so I merged it "as is".

          Show
          rowen Russell Owen added a comment - I didn't see any requested changes on github (or here) so I merged it "as is".

            People

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

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins

                No builds found.