DM-51599: Run crosstalk on CBP data / update and modernize CT code #355
DM-51599: Run crosstalk on CBP data / update and modernize CT code #355
Conversation
erykoff
left a comment
There was a problem hiding this comment.
I'm wondering if the units are all necessary, but I do see there are contracts to keep things in sync. I would think it could be inferred from the data though?
pipelines/LSSTCam/cpCrosstalk.yaml
Outdated
| cpCrosstalkExtract: | ||
| class: lsst.cp.pipe.CrosstalkExtractTask | ||
| config: | ||
| growMaskRadius: 20 |
There was a problem hiding this comment.
Should these just be in the parent cpCrosstalkLSST.yaml? Or these are so specific for the LSSTCam analysis?
There was a problem hiding this comment.
I measured growMaskRadius only on LSSTCam, and I suspect the value for LATISS would be different (but I haven't compared PSF wings between the two).
python/lsst/cp/pipe/cpCrosstalk.py
Outdated
| if np.sum(mask) == 0: | ||
| continue | ||
|
|
||
| newMask = np.where(np.bitwise_and(mask, detected), maskBit, 0) |
There was a problem hiding this comment.
This can be mask & detected which I find easier to read than np.bitwise_and().
python/lsst/cp/pipe/cpCrosstalk.py
Outdated
| outputFluxes=ddict2dict(outputFluxes) | ||
| ) | ||
|
|
||
| def _flipMask(self, maskArray, sourceAmp, targetAmp): |
There was a problem hiding this comment.
Don't we already have this code in ip_isr? Does it need to be rewritten, or can it be reused?
There was a problem hiding this comment.
The ip_isr version expects afw objects, and this uses numpy. It was different enough that I thought the copy-rewrite was worth it.
There was a problem hiding this comment.
Can you rename this flipMaskArray then?
python/lsst/cp/pipe/cpCrosstalk.py
Outdated
| if config.fluxOrder == 0: | ||
| self.inputs.discard("inputFluxes") | ||
| # if config.fluxOrder == 0 and False: | ||
| # self.inputs.discard("inputFluxes") |
There was a problem hiding this comment.
If these shouldn't be here, please remove.
python/lsst/cp/pipe/cpCrosstalk.py
Outdated
| myfluxes = np.ones_like(values) | ||
|
|
||
| if len(values) != len(myfluxes): | ||
| self.log.warning(f"Flux and ratio length disagree after first filter: {len(values)} {len(myfluxes)}") # noqa E501 |
There was a problem hiding this comment.
self.log.warning(
f"Flux and ratio length disagree after first filter: {len(values)} {len(myfluxes)}",
)
python/lsst/cp/pipe/cpCrosstalk.py
Outdated
| values = values[good] | ||
| myfluxes = myfluxes[good] | ||
| if len(values) != len(myfluxes): | ||
| self.log.warning(f"Flux and ratio length disagree after second filter: {len(values)} {len(myfluxes)}") # noqa E501 |
python/lsst/cp/pipe/cpCrosstalk.py
Outdated
| # filter those. | ||
| pass | ||
|
|
||
| itl_c0 = np.array(itl_c0) |
python/lsst/cp/pipe/cpCrosstalk.py
Outdated
|
|
||
| Parameters | ||
| ---------- | ||
| matrix0 : `np.array`, (Ndet, Namp, Namp) |
fe6697e to
ce85add
Compare
python/lsst/cp/pipe/cpCrosstalk.py
Outdated
| ) | ||
| growMaskRadius = Field( | ||
| dtype=int, | ||
| default=0, |
There was a problem hiding this comment.
I know you said above that you haven't measured this on anything but LSSTCam with CBP, but I still think having this as something non-zero as the default would make sense. Just set this to 20 here and delete the other overrides and we'll figure it out in the future if we need to?
python/lsst/cp/pipe/cpCrosstalk.py
Outdated
| outputFluxes=ddict2dict(outputFluxes) | ||
| ) | ||
|
|
||
| def _flipMask(self, maskArray, sourceAmp, targetAmp): |
There was a problem hiding this comment.
Can you rename this flipMaskArray then?
python/lsst/cp/pipe/cpCrosstalk.py
Outdated
| dtype=int, | ||
| default=0, | ||
| doc="Polynomial order in source flux to fit crosstalk.", | ||
| doc="Order of source flux fit to crosstalk. 0=simple linear; 1=first order non-linear.", |
There was a problem hiding this comment.
I'm getting confused as to why this isn't 1 and 2. What am I missing?
There was a problem hiding this comment.
This is the order of the fit done to ratio(source_flux) = target / source_flux. This removes one factor of the source_flux, so a 0-order fit is just a constant crosstalk ratio, and a 1-order fit is a linear fit of an "average" crosstalk ratio with a first-order source_flux correction. I agree the nomenclature is confusing.
python/lsst/cp/pipe/cpCrosstalk.py
Outdated
| values = np.array(ratios[ordering[tt]][ordering[ss]]) | ||
| values = values[np.abs(values) < 1.0] # Discard unreasonable values | ||
| values = np.asarray(ratios[ordering[tt]][ordering[ss]]) | ||
| good_values = np.abs(values) < 1.0 # Discard unreasonable values |
There was a problem hiding this comment.
What makes this unreasonable?
There was a problem hiding this comment.
This check is designed to remove ratios that have a target location with an actual star there. This translates to excluding all measured ratios that have absolute value greater than 1, such that target_flux > source_flux.
There was a problem hiding this comment.
Can you add a comment this in the code?
032d196 to
f61fbf3
Compare
a) check that flux and ratio length match b) pass fluxes to measurement code, to allow for preliminary non-linear fitting c) log clarifications d) Add stage subsets.
erykoff
left a comment
There was a problem hiding this comment.
A small comment, otherwise looks good. I also looked and I think that the filtering works even when limited to linear xtalk (e.g. it's looking for bad outliers with < and > and that should work).
python/lsst/cp/pipe/cpCrosstalk.py
Outdated
| values = np.array(ratios[ordering[tt]][ordering[ss]]) | ||
| values = values[np.abs(values) < 1.0] # Discard unreasonable values | ||
| values = np.asarray(ratios[ordering[tt]][ordering[ss]]) | ||
| good_values = np.abs(values) < 1.0 # Discard unreasonable values |
There was a problem hiding this comment.
Can you add a comment this in the code?
No description provided.