Add crosstalk to ISR #40
Conversation
python/lsst/ip/isr/crosstalk.py
Outdated
| minPixelToMask = Field(dtype=float, default=45000, | ||
| doc="Set crosstalk mask plane for pixels over this value") | ||
| crosstalkMaskPlane = Field(dtype=str, default="CROSSTALK", doc="Name for crosstalk mask plane") | ||
| coeffs = ListField( |
There was a problem hiding this comment.
I don't think the crosstalk coefficients can be in the config. This means you can only run one task per chip. It needs to be configurable from the dataRef.
python/lsst/ip/isr/crosstalk.py
Outdated
| dtype=float, | ||
| doc=("Flattened crosstalk coefficient matrix; should have nAmps x nAmps entries. " | ||
| "Once 'reshape'-ed, ``coeffs[i][j]`` is the fraction of the j-th amp present on the i-th amp."), | ||
| default=[0, 0, 0, 0, |
There was a problem hiding this comment.
This bakes in an assumption of a 2x2 configuration. I don't think having a default is very useful here.
| """Apply intra-CCD crosstalk correction""" | ||
| ConfigClass = CrosstalkConfig | ||
|
|
||
| def run(self, exposure): |
There was a problem hiding this comment.
I think that since the coefficients can't be in the config, this signature needs to take either a dataRef or an opaque object that contains the details of the crosstalk.
python/lsst/ip/isr/crosstalk.py
Outdated
| """ | ||
| mi = exposure.getMaskedImage() | ||
| mask = mi.getMask() | ||
| bg = calculateBackground(mi, badPixels) |
There was a problem hiding this comment.
Since this is before flatfield, shouldn't the background be calculated per amp?
python/lsst/ip/isr/crosstalk.py
Outdated
|
|
||
| # Set crosstalkStr bit only for those pixels that have been significantly modified | ||
| # (not necessarily those that are bright originally). | ||
| mask.clearMaskPlane(crosstalkPlane) |
There was a problem hiding this comment.
I'm missing something here. Doesn't this clear the mask plane? Where do we set the crosstalk bit?
There was a problem hiding this comment.
I'm curious about @SimonKrughoff 's comment here too.
PaulPrice
force-pushed
the
tickets/DM-11268
branch
4 times, most recently
from
5f175e8
to
59dc492
Compare
python/lsst/ip/isr/crosstalk.py
Outdated
|
|
||
| # Set crosstalkStr bit only for those pixels that have been significantly modified | ||
| # (not necessarily those that are bright originally). | ||
| mask.clearMaskPlane(crosstalkPlane) |
There was a problem hiding this comment.
I'm curious about @SimonKrughoff 's comment here too.
|
|
||
| Returns | ||
| ------- | ||
| ratios : `list` of `list` of `list` of `float` |
There was a problem hiding this comment.
list of list of list? Isn't there a better way to represent this?
There was a problem hiding this comment.
I can't think of a better description. As mentioned in the docstring, it's a matrix of lists of floats.
There was a problem hiding this comment.
Modified to "list of list of ndarray", with an explanation that it is a matrix of arrays.
| if ii == jj: | ||
| continue | ||
| jImage = extractAmp(mi.getImage(), jAmp, iAmp.getReadoutCorner()) | ||
| pixels = (jImage.getArray()[select] - bg)/iImage.getImage().getArray()[select] |
There was a problem hiding this comment.
How does this select apply to jImage, since it was created on iMask?
There was a problem hiding this comment.
The amps are assumed to be the same size, and I want to select the same pixels on jImage as on iImage.
| lo, med, hi = np.percentile(values, [25.0, 50.0, 75.0]) | ||
| sigma = 0.741*(hi - lo) | ||
| good = np.abs(values - med) < rejSigma*sigma | ||
| print(ii, jj, rej, med, sigma, len(values), good.sum()) |
There was a problem hiding this comment.
print statement here isn't going to be helpful for debugging: please remove it, or turn it into a log.debug(), with labels for the various bits.
| coeff : `numpy.ndarray` | ||
| Crosstalk coefficients. | ||
| coeffErr : `numpy.ndarray` | ||
| Crosstalk coefficient errors. |
There was a problem hiding this comment.
Mention that these are calculated as the std-dev of the mean?
| values = np.array(ratios[ii][jj]) | ||
| values = values[np.abs(values) < 1.0] # Discard unreasonable values | ||
| for rej in range(rejIter): | ||
| lo, med, hi = np.percentile(values, [25.0, 50.0, 75.0]) |
There was a problem hiding this comment.
Should this be using afw.stats or some other standard stats package instead?
There was a problem hiding this comment.
That would be best, but we don't have these robust stats in afw (yet?).
|
|
||
| @classmethod | ||
| def parseAndRun(cls, *args, **kwargs): | ||
| """Implement scatter/gather""" |
There was a problem hiding this comment.
Should probably document the return value here, since it's a tuple of things.
PaulPrice
force-pushed
the
tickets/DM-11268
branch
2 times, most recently
from
ebb9bf7
to
433fb85
Compare
Adapted from an implementation long used in obs_subaru for Suprime-Cam and Hyper Suprime-Cam; this was generalised to allow for an arbitrary number of amplifiers in arbitrary orientation, code cleaned up and docs updated. We provide both function and Task interfaces to both measure and remove the crosstalk.
Using the new CrosstalkTask. Crosstalk correction is disabled by default since this is new functionality that requires measured coefficients.
Tests both crosstalk measurement and removal, using both the function and Task interfaces.
PaulPrice
force-pushed
the
tickets/DM-11268
branch
from
433fb85
to
220888b
Compare
No description provided.