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
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.
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. 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.
Invalidated by RFC-687.
Krzysztof Findeisen
added a comment - Invalidated by RFC-687 .
Krzysztof Findeisen — I'm curious for your take on this. I think what you did on
DM-17566accurately 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?