DM-53418: Further robustify the pedestal background subtraction in calibrateImage #1197
Conversation
laurenam
force-pushed
the
tickets/DM-53418
branch
from
e70d7eb to
2a0c73f
Compare
laurenam
changed the title
laurenam
force-pushed
the
tickets/DM-53418
branch
3 times, most recently
from
bb4b5b6 to
fd4adfa
Compare
| # start an iteration with a small bin size, then double it on each | ||
| # iteration until the relative or absolute change criterion is met. | ||
| # If those are never achieved, the iteration stops when the bin size | ||
| # gets bigger than the exposure's bounding box. |
There was a problem hiding this comment.
I think this comment should be moved up to line 2036?
| self.log.info("Initial pedestal binSize = %d pixels", pedestalBackgroundConfig.binSize) | ||
| inPedestalIteration = True | ||
| cumulativePedestalLevel = 0.0 | ||
| relativeStoppingCriterion = 0.05 # Relative change in cumulative pedestal value. |
There was a problem hiding this comment.
This should have a longer comment (above it): "When the cumulative pedestal value changes by less than 5% from one bin size to the next we assume we are converged"
| inPedestalIteration = True | ||
| cumulativePedestalLevel = 0.0 | ||
| relativeStoppingCriterion = 0.05 # Relative change in cumulative pedestal value. | ||
| absoluteStoppingCriterion = 0.5 # Absolute change in cumulative pedestal value. |
There was a problem hiding this comment.
This should have a longer comment (above it): "When the cumulative pedestal value changes by less than 0.5 counts (electrons or adu) from one bin size to the next, we assume we are converged".
Meanwhile, is this the right convergence amount? Seems larger than I would naively expect, but you've run the numbers.
There was a problem hiding this comment.
Yeah, in all my testing, any smaller than this and it just started bouncing around at each iteration (which is what this is trying to avoid).
| pedestalBackgroundLevel, cumulativePedestalLevel, | ||
| relativeDelta, absoluteDelta) | ||
| if (relativeDelta > relativeStoppingCriterion | ||
| and absoluteDelta > absoluteStoppingCriterion): |
There was a problem hiding this comment.
I think this logic as it is written is hard to parse. Would it be clearer if it was (somewhat):
if relativeDelta < relativeStoppingCriterion or absoluteDelta < absoluteStoppingCriterion:
# We are converged
inPedestalIteration = False
else:
# We have not yet converged; grow the bin size if possible.
if binsize < blah:
binsize = newbinsize
log.info
# (No need to set this to True again)
else:
# We have reached the maximum binsize.
log.warning
inPedestalIteration = False
This adds an extra safeguard on the pedestal background measurement by adding an iteration on the measurement, increasing (doubling) the bin size at each stage. The bin size starts very small and there is at least one iteration. The stopping criterion is if either the change in the cumulative pedestal between the current and subsequent level is less than 5% of the current measured (cumulative) pedestal (relative change) or 0.5 "counts" (absolute change) or if the bin size has reached a value greater than the size of the exposure's bounding box.
laurenam
force-pushed
the
tickets/DM-53418
branch
from
fd4adfa to
7eccbd0
Compare
No description provided.