tickets/DM-17390 Update CalibrateTask to be a PipelineTask #264
Conversation
| @@ -166,9 +168,90 @@ class CalibrateConfig(pexConfig.Config): | |||
| doc="Injection of fake sources for testing purposes (must be " | |||
| "retargeted)" | |||
| ) | |||
| icSourceSchema = InitInputDatasetField( | |||
There was a problem hiding this comment.
What do you think about calling this inputSourceSchema (and similarly inputSourceCatalog), below, for more consistency with the output dataset field names?
python/lsst/pipe/tasks/calibrate.py
Outdated
| dimensions=("Instrument", "Visit", "Detector"), | ||
| scalar=True | ||
| ) | ||
| outputCatalog = OutputDatasetField( |
There was a problem hiding this comment.
Should standardize on either Cat or Catalog as the suffix for all of these, at least to the extent possible without breaking the current run signature.
| storageClass="Catalog", | ||
| dimensions=("Instrument", "Visit", "Detector"), | ||
| scalar=True | ||
| ) |
There was a problem hiding this comment.
It'd be good to have more consistency between the match dataset field names and the config options that control whether to produce them, too; maybe just "matches" and "matchesDenormalized"?
| self.pixelMargin = photoRefObjLoader.config.pixelMargin | ||
| # If this is gen 3 don't allow LoadAstometryNet loader | ||
| if initInputs is not None and photoRefObjLoader is LoadAstrometryNetObjectsTask: | ||
| raise RuntimeError("Astrometry Net tasks are not compatible with gen 3 middleware") |
There was a problem hiding this comment.
It would probably be good to add a few more comments to the logic here to note which code paths are expected for PipelineTask execution vs. CmdLineTask execution. We'll certainly appreciate that later when we come back to remove the latter.
python/lsst/pipe/tasks/calibrate.py
Outdated
| @@ -403,11 +507,10 @@ def runDataRef(self, dataRef, exposure=None, background=None, icSourceCat=None, | |||
| by icSourceKeys, or None; | |||
| @param[in] doUnpersist unpersist data: | |||
| - if True, exposure, background and icSourceCat are read from | |||
| dataRef and those three arguments must all be None; | |||
| dataRef and those three arguments must l be None; | |||
There was a problem hiding this comment.
Why did you change this? If the old documentation was incorrect, I don't see where.
python/lsst/pipe/tasks/calibrate.py
Outdated
| - if False the exposure must be provided; background and | ||
| icSourceCat are optional. True is intended for running as a | ||
| command-line task, False for running as a subtask | ||
|
|
||
| command-line task, False for running as a k |
There was a problem hiding this comment.
Something got mangled here; maybe the whole line change was accidental?
python/lsst/pipe/tasks/calibrate.py
Outdated
| refObjLoader = ReferenceObjectLoader(inputDataIds['astromRefCat'], | ||
| butler, | ||
| self.config.astromRefObjLoader, | ||
| self.log) |
There was a problem hiding this comment.
With four arguments in a call I'd recommend using kwargs (same below).
| expId, expBits = butler.registry.packDataId("VisitDetector", | ||
| inputDataIds['exposure'], | ||
| returnMaxBits=True) | ||
| inputData['exposureIdInfo'] = ExposureIdInfo(expId, expBits) |
There was a problem hiding this comment.
Probably a topic to discuss after the demo, but I wonder if it would be more readable to instead just pass extra keyword arguments (especially those that don't represent datasets) to run explicitly, rather than putting them in inputData.
python/lsst/pipe/tasks/calibrate.py
Outdated
|
|
||
| results = self.run(**inputData) | ||
|
|
||
| if self.config.doWriteMatches and results.astromMatches is not None: |
There was a problem hiding this comment.
Is it possible for the first clause here to be true while the second is false? If so, that could be a problem in the future (because we're lying to the activator about what it should expect to see in the outputs). If we can easily convince ourselves that if self.config.doWriteMatches alone is sufficient here now, that'd be ideal; if not, I'd like to at least add a warning if results.astromMatrches is unexpectedly None and open a ticket to follow up later.
python/lsst/pipe/tasks/calibrate.py
Outdated
| @@ -598,6 +734,10 @@ def run(self, exposure, exposureIdInfo=None, background=None, | |||
| sourceCat=sourceCat, | |||
| astromMatches=astromMatches, | |||
| matchMeta=matchMeta, | |||
|
|
|||
There was a problem hiding this comment.
Just seems like a weird place for a newline. I'd either drop it or replace it with comment headings if you want to define groups of output struct fields.
natelust
force-pushed
the
tickets/DM-17390
branch
from
7812595
to
6eb0698
Compare
natelust
force-pushed
the
tickets/DM-17390
branch
from
6eb0698
to
078d5c4
Compare
No description provided.