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

afwMath.Background.getImageF() unexpectedly ignores controller's undersample setting

    Details

    • Type: Story
    • Status: Invalid
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Team:
      Alert Production

      Description

      Historically, afwMath.BackgroundControl's constructor accepted the parameter style, to set the the interpolation style, and undersampleStyle, to set the behaviour when there are two few grid points for interpolation.

      This, to interpolate with an Akima spline, and to reduce the interpolation order when there aren't enough grid points, you'd do something like:

      bctrl = BackgroundControl("AKIMA_SPLINE", ..., undersampleStyle="REDUCE_INTERP_ORDER")
      background = makeBackground(image, bctrl)
      background.getImageF()
      

      On DM-17566, the style argument (but not the undersampleStyle argument) was deprecated, and, instead, the user is encouraged to instead call:

      background.getImageF(style, undersampleStyle="THROW_EXCEPTION")
      

      The confusion arises when the user does something like this:

      bctrl = BackgroundControl(undersampleStyle="REDUCE_INTERP_ORDER", ...)
      background = makeBackground(image, bctrl)
      background.getImageF("AKIMA_SPLINE")
      

      I contend that the reasonable user would be expecting this to use a REDUCE_INTERP_ORDER undersample style, as set in the BackgroundControl, but instead the getImageF default of THROW_EXCEPTION is used.

      This seems wrong to me.

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment -

            Krzysztof Findeisen — I'm curious for your take on this. I think what you did on DM-17566 accurately captured what the earlier deprecation notices in the code said to do, so I'm definitely not complaining about that... but this seems like a nasty side effect. Did you run across it?

            Show
            swinbank John Swinbank added a comment - Krzysztof Findeisen — I'm curious for your take on this. I think what you did on DM-17566 accurately captured what the earlier deprecation notices in the code said to do, so I'm definitely not complaining about that... but this seems like a nasty side effect. Did you run across it?
            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            I did not, but I didn't do any testing beyond the existing unit tests (some of which required manually adding some explicit keywords).

            I'm wondering if we shouldn't revert the Background-related deprecations (which are isolated in 2-3 commits of their own, as is right and proper). I'm pretty sure that, given the historical lack of any deprecation warnings, the people who planned out how to improve the Background API weren't fully aware of these kinds of consistency issues; it might be better to approach the problem from scratch, and then RFC the new solution.

            Show
            krzys Krzysztof Findeisen added a comment - - edited I did not, but I didn't do any testing beyond the existing unit tests (some of which required manually adding some explicit keywords). I'm wondering if we shouldn't revert the Background -related deprecations (which are isolated in 2-3 commits of their own, as is right and proper). I'm pretty sure that, given the historical lack of any deprecation warnings, the people who planned out how to improve the Background API weren't fully aware of these kinds of consistency issues; it might be better to approach the problem from scratch, and then RFC the new solution.
            Hide
            swinbank John Swinbank added a comment -

            Ok, thanks.

            I had a similar thought about reverting. Overall, though, I think that there were at least as many inconsistencies before removing the deprecated code as there will be after, and (following DM-23056) I don't think we'll be generating too many new and exciting warnings. Given that, I'm inclined to leave it as-is for now, and see if any unexpected issues arise over the rest of this release cycle.

            As for a comprehensive redesign — I'm pretty sure that's be planned before, and has never come to pass due to lack of time. I'd find it hard to prioritise it now, given that the code is basically working and is not blocking progress elsewhere, but we should certainly bear the possibility in mind when planning for the future.

            Show
            swinbank John Swinbank added a comment - Ok, thanks. I had a similar thought about reverting. Overall, though, I think that there were at least as many inconsistencies before removing the deprecated code as there will be after, and (following DM-23056 ) I don't think we'll be generating too many new and exciting warnings. Given that, I'm inclined to leave it as-is for now, and see if any unexpected issues arise over the rest of this release cycle. As for a comprehensive redesign — I'm pretty sure that's be planned before, and has never come to pass due to lack of time. I'd find it hard to prioritise it now, given that the code is basically working and is not blocking progress elsewhere, but we should certainly bear the possibility in mind when planning for the future.
            Hide
            krzys Krzysztof Findeisen added a comment -

            Invalidated by RFC-687.

            Show
            krzys Krzysztof Findeisen added a comment - Invalidated by RFC-687 .

              People

              • Assignee:
                Unassigned
                Reporter:
                swinbank John Swinbank
                Watchers:
                John Swinbank, Krzysztof Findeisen
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel