Update ImageDifferenceTask to remove v20 deprecation warnings

XMLWordPrintable

Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s: None
• Labels:
None
• Story Points:
1
• Team:
• Urgent?:
No

Description

 \$ imageDifference.py DATA/ --rerun ci_hsc:gen2cmp_2020-01-31 -c doSelectSources=False --id visit=903334 ccd=22 filter=HSC-R |& tee -a imgDiff_gen2_hsc_2020-01-31.log     /ssd/gkovacs/devel/active/DM-22541_ip_diffim/python/lsst/ip/diffim/imagePsfMatch.py:785: FutureWarning: Call to deprecated method getImageF(). (Zero-argument overload is deprecated; use one that takes an interpStyle instead. To be removed after 20.0.0.)   bkgd = self.background.fitBackground(mi).getImageF()  /ssd/gkovacs/devel/active/DM-22541_pipe_tasks_gen3_run/python/lsst/pipe/tasks/imageDifference.py:843: FutureWarning: Call to deprecated method makeSourceCatalog. (Replaced by SourceDetectionTask.run(). Will be removed after v20.)   doSmooth=not self.config.doPreConvolve 

Attachments

1. imgDiff_gen2_hsc_2020-02-26.log
5 kB

Activity

Hide
John Swinbank added a comment -

We ought to remove these before v20 is released. Is it easy? If so, let's just do it!

Show
John Swinbank added a comment - We ought to remove these before v20 is released. Is it easy? If so, let's just do it!
Hide
Gabor Kovacs added a comment - - edited

Hmm... though this seemed trivial and somewhat unimportant (related to the kernel source selection), it scratches the API change of DM-22814.

Krzysztof Findeisen In ip_diffim there is a direct call of SubtractBackgroundTask.fitBackground(). Is this the way to replace getImage(void) in this context ? It's ugly because it refers to the subtask config from the outside.

 --- a/python/lsst/ip/diffim/imagePsfMatch.py +++ b/python/lsst/ip/diffim/imagePsfMatch.py @@ -782,7 +782,9 @@ class ImagePsfMatchTask(PsfMatchTask):  maskArr = mi.getMask().getArray()  miArr = np.ma.masked_array(imArr, mask=maskArr)  try: - bkgd = self.background.fitBackground(mi).getImageF() + fitBg = self.background.fitBackground(mi) + bkgd = fitBg.getImageF(self.background.config.algorithm, + self.background.config.undersampleStyle)  except Exception:  self.log.warn("Failed to get background model. Falling back to median background estimation")  bkgd = np.ma.extras.median(miArr)

Alternatively, we could add a getBackgroundAsMaskedImage() method to SubtractBackgroundTask as part of the API change to avoid accessing the config from the outside. What do you think?

Show
Hide
Krzysztof Findeisen added a comment -

Oh dear. For the record, I'm not an expert on ip_diffim, meas_algorithms, or even afw.image.Background, so take everything I say here as speculative.

I think the fundamental problem is that SubtractBackgroundTask was designed on the assumption that a Background is a self-contained entity, one of whose properties is the interpolation style (what SubtractBackgroundTask confusingly calls algorithm). The Background API changes were designed on the assumption that a Background is a logically distinct entity from its image representation, and that interpolation/undersampling only matters when you want such a representation.

Honestly, I think this is the best evidence yet that the Background changes were not very well thought-out, and that we should roll back the corresponding deprecations until we can do a proper redesign (even if we never get to it).

If we're not taking that route, then in the long term we would need to reconcile SubtractBackgroundTask's and Background's interpretations of what a background is. John Swinbank and I deprecated the algorithm keyword to fitBackground on the grounds that it would become useless once the old Background API was removed... but what happens to config.algorithm and config.undersampleStyle? Will they end up having no effect on SubtractBackgroundTask? If so, the most natural solution might be to move them to ImagePsfMatchTask and any other classes that care about background interpolation... but that's a fairly ambitious change.

I guess I'd recommend the self.background.config approach as the option that gives us the most flexibility, because it keeps the "ugliness" inside implementation code. Changing SubtractBackgroundTask's API for the convenience of a specific client seems like a step in the wrong direction.

Show
Krzysztof Findeisen added a comment - Oh dear. For the record, I'm not an expert on ip_diffim , meas_algorithms , or even afw.image.Background , so take everything I say here as speculative. I think the fundamental problem is that SubtractBackgroundTask was designed on the assumption that a Background is a self-contained entity, one of whose properties is the interpolation style (what SubtractBackgroundTask confusingly calls algorithm ). The Background API changes were designed on the assumption that a Background is a logically distinct entity from its image representation, and that interpolation/undersampling only matters when you want such a representation. Honestly, I think this is the best evidence yet that the Background changes were not very well thought-out, and that we should roll back the corresponding deprecations until we can do a proper redesign (even if we never get to it). If we're not taking that route, then in the long term we would need to reconcile SubtractBackgroundTask 's and Background 's interpretations of what a background is. John Swinbank and I deprecated the algorithm keyword to fitBackground on the grounds that it would become useless once the old Background API was removed... but what happens to config.algorithm and config.undersampleStyle ? Will they end up having no effect on SubtractBackgroundTask ? If so, the most natural solution might be to move them to ImagePsfMatchTask and any other classes that care about background interpolation... but that's a fairly ambitious change. I guess I'd recommend the self.background.config approach as the option that gives us the most flexibility, because it keeps the "ugliness" inside implementation code. Changing SubtractBackgroundTask 's API for the convenience of a specific client seems like a step in the wrong direction.
Hide
John Swinbank added a comment -

I would support an RFC to un-deprecate the old Background API.

Failing that, I think the self.background.config approach is what I've used elsewhere, and it's fine, if ugly.

Show
John Swinbank added a comment - I would support an RFC to un-deprecate the old Background API. Failing that, I think the self.background.config approach is what I've used elsewhere, and it's fine , if ugly.
Hide
Gabor Kovacs added a comment - - edited

Ugly changes implemented then. Test run log attached, seems to be ok.

Show
Gabor Kovacs added a comment - - edited Ugly changes implemented then. Test run log attached, seems to be ok.
Hide
Gabor Kovacs added a comment -

John Swinbank Would you please review this ticket?

Show
Gabor Kovacs added a comment - John Swinbank Would you please review this ticket?
Hide
John Swinbank added a comment -

Somewhat hesitant, because I'm still on the fence about whether Krzysztof is right and we should just unwind all of this.

But for now, I think this is fine. Thanks Gabor, ok to merge.

Show
John Swinbank added a comment - Somewhat hesitant, because I'm still on the fence about whether Krzysztof is right and we should just unwind all of this. But for now, I think this is fine. Thanks Gabor, ok to merge.

People

• Assignee:
Gabor Kovacs
Reporter:
Gabor Kovacs
Reviewers:
John Swinbank
Watchers:
Gabor Kovacs, John Swinbank, Krzysztof Findeisen, Yusra AlSayyad