Details
-
Type:
Improvement
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: meas_algorithms
-
Labels:None
-
Story Points:4
-
Epic Link:
-
Sprint:Alert Production X16 - 5
-
Team:Alert Production
Description
Implement RFC-155: change estimateBackground as follows:
- Always subtract the background
- Modify the exposure in place
- Replace estimateBackground with the run method of a new task SubtractBackgroundTask
- Replace getBackground (which fits a background) with SubtractBackgroundTask.fitBackground
Attachments
Issue Links
Activity
Review completed on GitHub PRs. Besides some little things, the only major comment I have is that the SubtractBackgroundTask.run method should be made as compact as possible by delegating work to other methods.
Paul Price
added a comment - Review completed on GitHub PRs. Besides some little things, the only major comment I have is that the SubtractBackgroundTask.run method should be made as compact as possible by delegating work to other methods. Thank you for the comprehensive review. It was very helpful.
I agree that computing statistics on the background is a poor idea for this task, but it appears to be necessary for now, so I carried it over from existing code and filed RFC-180. I also agree that the background subtraction in `ip_diffim` could be cleaned up a bit, but I feel that would be best left for a more comprehensive refactoring. I was wrong about the tight coupling between the task's config and `makeBackground`'s control object, so I updated the documentation accordingly.
Aside from that, I believe I implemented all of your suggested changes. I also modernized the display code in `SubtractBackgroundTask` and rebased from master. Once CI passes I will merge.
The changes are described here:
https://community.lsst.org/t/changes-in-how-background-subtraction-is-done/756
Most of the work is in meas_algorithms, but it affected some code in ip_diffim, pipe_drivers and pipe_tasks. It also required a trivial change to a comment in meas_base.