Details
-
Type:
RFC
-
Status: Retired
-
Resolution: Done
-
Component/s: DM
-
Labels:None
-
Location:Issue comments
Description
During the S14 efforts to document the important PipeTasks, it was pointed out that the only way to use some of the low level tasks is to supply a ButlerDataRef. This discourages reuse and requires code examples/tests to create a fake ButlerDataRef. This RFC proposes to add a non-blob interface to IsrTask specficially (epic: DM-1113). Currently, the commandlineTask processCcd constructs a DataRef from the commandline arguments. processCcd.run() calls isrTask.run(dataRef) which retrieves the calibration data and removes the instrument signature.
Proposal: Keep the responsibility of retrieving the calibration data in IsrTask (and more generally always give lower-level tasks (e.g. FringeTask) responsibility of reading necessary data from the butler). Split run(sensorRef) into 3 methods:
- run(sensorRef) which wraps a two new methods
- readDetrendData(dataRef)} and
- doIsr(ccdExposure, biasExposure, darkExposure, flatExposure, defects, fringes).
See ip_isr branch /u/yusra/DM-1299 for a prototype or pseudocode example below.
A benefit to having a separate method to read the data is the hierarchical symmetry with lower-level subtasks of IsrTask such as FringeTask. This proposal will also require an analogous refactoring of FringeTask.
A complication is that IsrTask is the most camera-specific task in the stack and requires a lot of input. readDetrendData and doIsr (if not run too) will likely need to be overridden in obs_*. In obs_lsstSim for example, run could also be responsible for combining snaps.
This change should not break any client code, but will require an analogous refactoring of the obs_* IsrTasks, and a clean up of examples and unit tests that create fake dataRefs. Also note the choice of method names run/doIsr instead of runDataRef/run. Rational was that run will still be the primary method called and this maintains backwards compatibility.
Pseudocode example
class IsrTask:
|
...
|
def readDetrendData(self, sensorRef):
|
biasExposure = self.getDetrend(sensorRef, "bias")
|
...
|
if self.config.doFringe:
|
fringes = self.fringe.readFringes(sensorRef, assembler=self.assembleCcd)
|
...
|
return dict of calibration products {biasExposure, ... fringes}
|
|
|
def doIsr(self, ccdExposure, biasExposure=None, darkExposure=None, flatExposure=None,
|
defects=None, fringes=None):
|
#do processing including subtasks, e.g.
|
if self.config.doBias:
|
self.biasCorrection(exp, biasExp)
|
...
|
if self.config.doFringe:
|
self.fringe.removeFringe(ccdExposure, fringeData, assembler=None)
|
...
|
|
|
def run(self, sensorRef):
|
ccdExposure = sensorRef.get('raw')
|
detrendData = self.readDetrendData(sensorRef)
|
ccdExposure = self.doIsr(ccdExposure, **detrendData).exposure
|
...
|
|
|
class FringeTask:
|
def readFringes(self, dataRef, assembler=None):
|
...
|
def removeFringe(self, exposure, fringes, assembler=None):
|
...
|
def run(self, exposure, dataRef, assembler=None):
|
...
|
fringes = self.readFringes(dataRef, assembler=assembler)
|
self.removeFringe(exposure, fringes, assembler=assembler)
|
Attachments
Issue Links
Activity
I think I'd rather keep the run() task as the unit of reuse, so this would become
class IsrTask: |
...
|
def readDetrendData(self, sensorRef): |
biasExposure = self.getDetrend(sensorRef, "bias") |
...
|
if self.config.doFringe: |
fringes = self.fringe.readFringes(sensorRef, assembler=self.assembleCcd) |
...
|
return dict of calibration products {biasExposure, ... fringes} |
|
def run(self, ccdExposure, biasExposure=None, darkExposure=None, flatExposure=None, |
defects=None, fringes=None): |
#do processing including subtasks, e.g. |
...
|
|
def runFromDataref(self, sensorRef): |
ccdExposure = sensorRef.get('raw') |
detrendData = self.readDetrendData(sensorRef) |
ccdExposure = self.run(ccdExposure, **detrendData).exposure |
...
|
(and why is the variable called sensorRef not the more generic dataRef? Is it important that this be a sensorRef? if so, it'd be `runFromSensorRef')
I'm good with the run/runFromSensorRef method names you suggest. Both obs_lsstSim and obs_subaru, IsrTasks operate on a sensor-level. They read in each channel, do some processing, assemble the channels into a sensor-shaped exposure, and do some more processing. Which calibration products are stored at the channel-level and the sensor-level differs per camera.
This means that my original proposed names for the arguments to run are too specific, since it won't necessarily be a bias/flat/dark "Exposure". It might be sensor-shaped Exposure for some cameras or a dict/struct of channel-shaped Exposures for other cameras. I'm now thinking of the more flexible names:
def run(self, raw, bias=None, dark=None, flat=None, defects=None, fringes=None):
|
I like your proposal. I've come to believe that "run" makes more sense for command-line tasks, and for other tasks I prefer the methods be named after what they do, so in this case I'd suggest doIsr(self, raw, ...) and doIsrWithDataRef(self, dataRef). However, I won't push hard. Having the two methods is the main thing.
I agree with Paul Price that the term detrend is best avoided – likely replaced by ISR.
Hsin-Fang Chiang can you clarify the status of this RFC please? I see you added a ticket triggered by this RFC. This implies the status of the ticket should really be ADOPTED rather than RETIRED. Is the RFC being resurrected?
This came up in DM-6640 (please see the comment there). I guess I misused the issue link of "triggering" so am changing it to “relates to”.
Hsin-Fang Chiang
added a comment - This came up in DM-6640 (please see the comment there). I guess I misused the issue link of "triggering" so am changing it to “relates to”.
The bias, dark, flat, fringe, etc., should not be read unless they're going to be applied. Note that the fringe subtraction is not necessarily performed if doFringe is True, but it also looks at the filter.
Use a Struct instead of a dict for the output of readDetrendData.
The word "detrend" was inherited from Pan-STARRS and CFHT, but it hasn't historically been part of LSST-speak. This might be a good time to replace it with whatever LSST uses ("calib"?).