Changes in how background subtraction is done

Data Management DM Notifications
#1

As a result of DM-5323, which implements RFC-155, background estimation in Python is now done using different routines in meas_algorithms:

  • There is a new task SubtractBackgroundTask, with full documentation and a working example.
  • The existing function getBackground (which fit a background) is replaced by SubtractBackgroundTask.fitBackground. Changes from getBackground are:
    • getBackground could return None if the fit failed; in that situation fitBackround will raise RuntimeError instead of returning None
    • The argument image was renamed to maskedImage, for clarity
    • The config is not passed as an argument
    • The debug display code uses different keys and is updated to use afw.display
  • The existing function estimateBackground (which subtract a background from an exposure) is replaced by SubtractBackgroundTask.run. Changes from estimateBackground are:
    • You may pass in a background model (an lsst.afw.math.BackgroundList)
    • It returns a struct containing the updated background model
    • The config is not passed as an argument
    • The debug display code displays the unsubtracted image and uses different keys and is updated to use afw.display
  • The task’s config SubtractBackgroundConfig replaces the old old BackgroundConfig, with one small change:
    • The field algorithm may no longer be None; you must use the string "NONE", instead
#2

This seems like an undesirable change, so I assume it was forced by an implementation detail. Can you explain a bit?

#3

I don’t think it was a particularly good feature; this value is passed to C++ code, which expects string values (of which “NONE” is one supported and documented value).

Furthermore, as the reviewer @price pointed out, the substitution was made in the validate method, which should not change values. If we are going to restore it, it should be restored in some other way.

#4

Fair enough. I’m willing to count this as just a different workaround for an old problem instead of a new problem. Could you create a ticket for dealing with this in the future?

#5

@jbosch I’m afraid I don’t know what you are concerned about. If a config parameter takes a set of possible string values, one of which happens to be “NONE” then that’s what it takes. It is a convenience to accept None as a synonym for “NONE” in Python, but that is not necessarily a good thing, since it adds a second way to specify that particular value, which adds complexity.

If you are really keen to allow that particular synonym then it can probably be fixed in some cleaner way than messing with it in the validate method. If you see support for synonyms as important for configs in general (and I hope not) then the correct solution is probably adding a new standard method to Config that cleans up field values before use. However, I hope it doesn’t come to that.

#6

I’d say it’s a pretty strong expectation of a Pythonic interface that None be usable when “none” is conceptually an option.

I don’t have a strong opinion on the implementation, other than agreeing that changing values in validate is bad. What’s best probably depends on the details of the C++ class and how it’s called from Python.

#7

While Configs are Python as syntax, it’s not totally clear that Python None should be accepted within them, particularly for a field that is otherwise a string.