DM-6640: IsrTask is not a valid CmdLineTask #65
Conversation
python/lsst/ip/isr/isrTask.py
Outdated
| @anchor IsrWrapperTask_ | ||
|
|
||
| @brief Task to wrap the main IsrTask to allow camera specific retargeting. | ||
| """ |
There was a problem hiding this comment.
Please make the new docstrings numpydoc, otherwise we'll just have to convert them later.
Also, can you please expand on this documentation a bit? I don't really understand what makes it different from the original IsrTask.
There was a problem hiding this comment.
It wraps IsrTask, so that it can be retargeted.
There was a problem hiding this comment.
Docstrings updated to numpydoc. I didn't use a sufficiently new comparison to get the new format.
Docstring also amended to include information on what this change does, and why it's needed.
python/lsst/ip/isr/isrTask.py
Outdated
| @@ -1225,3 +1225,50 @@ def getSaturation(self): | |||
|
|
|||
| def getSuspectLevel(self): | |||
| return float("NaN") | |||
|
|
|||
|
|
|||
| class IsrWrapperTaskConfig(pexConfig.Config): | |||
There was a problem hiding this comment.
And double check that you update the config file name to match this in obs_base too
python/lsst/ip/isr/isrTask.py
Outdated
| @anchor IsrWrapperTask_ | ||
|
|
||
| @brief Task to wrap the main IsrTask to allow camera specific retargeting. | ||
| """ |
There was a problem hiding this comment.
It wraps IsrTask, so that it can be retargeted.
python/lsst/ip/isr/isrTask.py
Outdated
| @anchor IsrWrapperTask_ | ||
|
|
||
| @brief Task to wrap the main IsrTask to allow camera specific retargeting. | ||
| """ |
python/lsst/ip/isr/isrTask.py
Outdated
| @param[in] *kwargs a dictionary of keyword arguments passwed on to the | ||
| Task constructor | ||
| Call the lsst.pipe.base.task.Task.__init__ method, | ||
| then setup the default IsrTask as a subtask |
There was a problem hiding this comment.
You don't need this explanation: that's exactly what the code says, and the code says it in fewer words.
There was a problem hiding this comment.
Agree with earlier comments re. numpydoc, also please note our guide explicitly says not to have a docstring for init methods. More details at https://developer.lsst.io/python/numpydoc.html#py-docstring-class-structure
python/lsst/ip/isr/isrTask.py
Outdated
| """!Perform instrument signature removal on a ButlerDataRef of a Sensor | ||
| Parameters | ||
| ---------- | ||
| dataRef : `daf.persistence.butlerSubset.ButlerDataRef` |
There was a problem hiding this comment.
Should be:
dataRef : `lsst.daf.persistence.ButlerDataRef`
(Needs to be fully-qualified, but you can drop the butlerSubset because ButlerDataRef gets pulled into the parent namespace.)
python/lsst/ip/isr/isrTask.py
Outdated
| Parameters | ||
| ---------- | ||
| dataRef : `daf.persistence.butlerSubset.ButlerDataRef` | ||
| DataRef of the detector data to be processed |
There was a problem hiding this comment.
Data reference of the ...
python/lsst/ip/isr/isrTask.py
Outdated
|
|
||
| Returns | ||
| ------- | ||
| result : |
bin.src/runIsr.py
Outdated
| #!/usr/bin/env python | ||
| # | ||
| # LSST Data Management System | ||
| # Copyright 2008, 2009, 2010 LSST Corporation. |
There was a problem hiding this comment.
I don't think we need the copyleft here, do we? If so, fix the year and the copyright owner (Trustees of Princeton University).
There was a problem hiding this comment.
We do generally have copylefts in our scripts in bin.src/
More info:
- https://developer.lsst.io/stack/license-and-copyright.html
- https://github.com/lsst/templates/tree/master/file_templates/copyright
The new-style preamble assumes you have COPYRIGHT file in the repo.
There was a problem hiding this comment.
We should put the standard boiler plate (which changed recently so check in https://developer.lsst.io/stack/license-and-copyright.html?highlight=copyright ) in each source file. Copyrights should be in a COPYRIGHT file though, not each file.
There was a problem hiding this comment.
I found the updated boiler plate today while looking, so that has been updated as well.
bin.src/runIsr.py
Outdated
| # | ||
| from lsst.ip.isr.isrTask import IsrWrapperTask | ||
|
|
||
| IsrWrapperTask.parseAndRun() |
There was a problem hiding this comment.
Might just be me, but I would expect this to tun RunIsrTask given the name of the script is runIsr. Remember all the eye-rolling over bin/measureCoaddSources.py calling
MeasureMergedCoaddSourcesTask.parseAndRun() and the like in pipe_tasks
There was a problem hiding this comment.
I am open to different solutions, but I don't see one that doesn't involve either 1) making separate runIsr_obsX.py scripts for each obs_package; or 2) making the current RunIsrTask into RunIsrRealTask or something equivalent.
There was a problem hiding this comment.
Is there already a RunIsrTask???
There was a problem hiding this comment.
The pattern now should be IsrTask + camera-specific IsrTask's like DecamNullIsrTask. To my knowledge, there is no RunIsrTask. I'm going offline for the night. I don't care enough for you to change it to RunIsrTask, but be aware one of the PipelineTask precursor/refactoring jobs that @natelust going to work on is making the multiband script names match their Task names in pipe_tasks.
czwa
force-pushed
the
tickets/DM-6640
branch
from
fa8f608
to
7809aa2
Compare
python/lsst/ip/isr/isrTask.py
Outdated
| _DefaultName = "isrWrap" | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| pipeBase.Task.__init__(self, *args, **kwargs) |
There was a problem hiding this comment.
I think @natelust is or shortly will be going through the Tasks and making them use super() in places like this, so we can use multiple inheritance if we need to during the CmdLineTask->PipelineTask transition. Might be good to go ahead and do that in new code right now.
czwa
force-pushed
the
tickets/DM-6640
branch
from
7809aa2
to
12a9d3f
Compare
The IsrTask was not usable as a CmdLineTask, as it was written to be called as a subtask of ProcessCcdTask. This allows the default IsrTask to be retargeted to camera-specific versions of the ISR processing. A new RunIsrTask now exists to hold IsrTask for retargetting in standalone processing, and both processCcd.py and runIsr.py now read from the same configuration files for each obs_ package. This has involved migrating configuration values from config/processCcd.py to config/isr.py for many obs_ packages.
czwa
force-pushed
the
tickets/DM-6640
branch
from
12a9d3f
to
436b4c3
Compare
This ticket adds a stand-alone runIsr.py script, as well as ensuring that runIsr.py and processCcd.py read the same ISR configuration files.