DM-10299: Apply crosstalk correction in decam #77
Conversation
| # The xtalk file has image areas like 'ccd01A'; preserve only '01A' | ||
| sources[elem[0][3:]].append(elem[1][3:]) | ||
| # The linear crosstalk coefficients | ||
| coeffs[(elem[0][3:], elem[1][3:])] = float(elem[2]) |
There was a problem hiding this comment.
Am I reading this that you are not applying the non-linear terms of the crosstalk? I know this caused problems with the edges of saturated areas sneaking through.
There was a problem hiding this comment.
That's correct, I'm only applying the linear terms. That's what both the DES and THELI pipelines do, and I didn't see any edge problems by eye (doesn't mean they aren't there!). Do you think higher order coefficients are important to include?
There was a problem hiding this comment.
I really can't say how important they are. I was just echoing what I had read in docs from the DECam team. If they don't apply them, I don't see any reason we should.
python/lsst/obs/decam/crosstalk.py
Outdated
| from .isr import DecamIsrTask | ||
| decamisr = DecamIsrTask() | ||
| decamisr.overscanCorrection(exposure, amp) | ||
| victim = ccdnum_str + amp.getName() |
There was a problem hiding this comment.
This has already happened on line 152, right?
python/lsst/obs/decam/isr.py
Outdated
| @@ -47,13 +50,23 @@ class DecamIsrConfig(IsrTask.ConfigClass): | |||
| doc="Number of edge pixels to be flagged as untrustworthy.", | |||
| default=35, | |||
| ) | |||
| doCrosstalk = pexConfig.Field( | |||
There was a problem hiding this comment.
doCrosstalk and crosstalk should be in a config. We have the config mechanism to avoid just this sort of thing. In the config file you can retarget the crosstalk sub-task and set the value of doCrosstalk.
There was a problem hiding this comment.
I was trying to follow the convention used by other decam ISR config things by putting these here. Would it be clearer to have a new config/isr.py that has all of the DecamIsrConfig stuff in it? The IsrTask is already retargeted to DecamIsrTask in config/processCcd.py (and config/processCcdCpIsr.py).
There was a problem hiding this comment.
I think we should use the config override mechanism where possible rather than adding code. Then only DECam specific configs go in new code.
python/lsst/obs/decam/crosstalk.py
Outdated
| _DefaultName = 'decamCrosstalk' | ||
|
|
||
| @pipeBase.timeMethod | ||
| def run(self, exposure, dataRef): |
There was a problem hiding this comment.
I agree with Paul that this should be refactored to have a signature like: def run(self, exposure, source_dict) where source_dict is a dict possibly keyed by the source id string containing a tuple (or dict) of source exposure and coefficient.
There was a problem hiding this comment.
Here is an (untested) gist showing what I'm talking about: https://gist.github.com/SimonKrughoff/12b8bbb2e17a6cfe1b8cc4397d293410
There was a problem hiding this comment.
Note that this just has a prepCrosstalk method that you call in IsrTask.runDataRef.
python/lsst/obs/decam/crosstalk.py
Outdated
| for source in sources[victim]: | ||
| source_ccdnum = int(source[:2]) | ||
| try: | ||
| source_exposure = butler.get('raw', dataId={'visit': visit, 'ccdnum': source_ccdnum}) |
There was a problem hiding this comment.
You have all the information you need to do this outside this method. I think you could have a runDataRef method that assembles the data necessary for each victim as it comes along. It may have to grab all the sources for a full exposure, but then passes that structure on to run.
At that point, you may be able to just collapse this into the run method since all the data wrangling will be done in runDataRef.
mrawls
force-pushed
the
tickets/DM-10299
branch
from
1c54a36
to
f7d718e
Compare
config/processCcd.py
Outdated
| config.isr.retarget(DecamIsrTask) | ||
| config.isr.crosstalk.retarget(DecamCrosstalkTask) |
There was a problem hiding this comment.
You shouldn't have to retarget in both places. I think it might be better to not have a separate isr.py and just put those overrides here.
There was a problem hiding this comment.
Hmmm is this because you have two different processCcd-ish configs? If that's the case, I would like to see the dups removed and just use isr.py for everything.
There was a problem hiding this comment.
Right, it's because there are configs for both processCcd and processCcdCpIsr. Both contain more than just ISR-related config settings, however. The workflow for processing outlined in the obs_decam README uses them. I added the ISR config so the test I wrote which runs ISR only will work.
There was a problem hiding this comment.
Nevermind, got it. Didn't realize one could load configs from configs... or as Colin put it, import pexInception 😆
python/lsst/obs/decam/isr.py
Outdated
| """ | ||
| isrData = super().readIsrData(dataRef, exposure) | ||
| if self.config.doCrosstalk: | ||
| crosstalkSources = self.crosstalk.prepCrosstalk(dataRef) |
There was a problem hiding this comment.
I'd like to see this in the base class IsrTask so that other instruments can use this mechanism without reinventing wheels.
mrawls
force-pushed
the
tickets/DM-10299
branch
from
33fdd1b
to
96a49f2
Compare
There are two main functionalities: 1. part of the ISR workflow (primary) and 2. standalone crosstalk removal of a raw DECam FITS file
Update docstrings, clarify columns in decam xtalk coefficient file, make CrosstalkTask a Task rather than a CommandLineTask.
Also remove unnecessary "extensions" config dict
This makes a custom readIsrData unnecessary for DecamCpIsrTask, because ip_isr's readIsrData now uses these instead of hardwired 'bias' and 'flat' for all cases.
This new method returns crosstalkSources, which contain source image data and the corresponding crosstalk coefficient. The processCcd and processCcdCpIsr configs now retarget to DecamCrosstalkTask, and a new plain isr config does the same.
mrawls
force-pushed
the
tickets/DM-10299
branch
from
96a49f2
to
fd4b988
Compare
No description provided.