tickets/DM-16873 #255
tickets/DM-16873 #255
Conversation
python/lsst/pipe/tasks/multiBand.py
Outdated
| @@ -630,9 +632,85 @@ class MeasureMergedCoaddSourcesConfig(Config): | |||
| target=CatalogCalculationTask, | |||
| doc="Subtask to run catalogCalculation plugins on catalog" | |||
| ) | |||
| inputSchema = InitInputDatasetField( | |||
| doc="Input schema for measure merged task", | |||
There was a problem hiding this comment.
Probably better to document this by giving an example of a task that produces it; it's already obvious from context that it's the input schema for this task.
python/lsst/pipe/tasks/multiBand.py
Outdated
| storageClass="SourceCatalog" | ||
| ) | ||
| refCat = InputDatasetField( | ||
| doc="Reference catalog to use", |
There was a problem hiding this comment.
It's not obvious at the outset what the ref_cat is going to be used for in this task. Would be good to document that at some level.
python/lsst/pipe/tasks/multiBand.py
Outdated
| ) | ||
| visitCatalogs = InputDatasetField( | ||
| doc="Source catalogs for visits which overlap input tract, patch, abstract_filter. Will be " | ||
| "further filtered in the task", |
There was a problem hiding this comment.
I would also add something along the lines of "for the purpose of propagating flags from image calibration and characterization to coadd objects".
python/lsst/pipe/tasks/multiBand.py
Outdated
| doc=("Name of the input catalog to use." | ||
| "If the single band deblender was used this should be 'deblendedFlux." | ||
| "If the multi-band deblender was used this should be 'deblendedModel." | ||
| "If no deblending was performed this should be 'mergeDet'"), |
There was a problem hiding this comment.
I think the situation is slightly more complex than this (maybe @fred3m can confirm): if the multi-band deblender was used, I think this can be either deblendedFlux or deblendedModel.
| self.match.setRefObjLoader(refObjLoader) | ||
|
|
||
| # Set psfcache | ||
| inputData['exposure'].getPsf().setCacheCapacity(self.config.psfCache) |
There was a problem hiding this comment.
Could this be done in run instead?
There was a problem hiding this comment.
In general yes this sort of thing could go in run. However in this case, runDataRef has a specific interface for this that I didn't want to change, so it is here so I don't step on gen2 toes. I will add a comment to move it to run after gen2 deprecation.
python/lsst/pipe/tasks/multiBand.py
Outdated
| inputCatalogWcsUpdate = [] | ||
| for (i, dataId) in enumerate(inputDataIds['visitCatalogs']): | ||
| key = (dataId['visit'], dataId['detector']) | ||
| if (key) in inputVisitIds: |
| inputCatalogWcsUpdate.append(ccdRecordsWcs[key]) | ||
| inputData['visitCatalogs'] = inputCatalogsToKeep | ||
| inputData['wcsUpdates'] = inputCatalogWcsUpdate | ||
| inputData['ccdInputs'] = ccdInputs |
There was a problem hiding this comment.
Hopefully we can clean this up more fully when we don't have to support the CmdLineTask versions of things anymore, but do you think it would be an improvement now to bundle these three arguments into (e.g.) a namedtuple or something.
There was a problem hiding this comment.
Since ccdInputs is in the interface for the existing propagate visit flags, this is something I dont want to bundle as it would just need to be unpacked later. That leaves 2 things which does not seem worth bundling. I think it is better to have an explicit function signature instead of packing things just to unpack them inside the function.
|
|
||
| self.measurement.run(sources, exposure, exposureId=self.getExposureId(patchRef)) | ||
| def run(self, exposure, sources, skyInfo, exposureId, ccdInputs=None, visitCatalogs=None, wcsUpdates=None, | ||
| butler=None): |
There was a problem hiding this comment.
I know we're just trying to get things done quickly right now and plan to clean up in the future, but adding parameter docs for this method might be worth doing on this ticket, especially for anyone who might need to make changes to the Gen2 version before we retire it.
| @@ -165,28 +166,23 @@ def run(self, butler, coaddSources, ccdInputs, coaddWcs): | |||
| @param[in,out] coaddSources Source catalog from the coadd | |||
| @param[in] ccdInputs Table of CCDs that contribute to the coadd | |||
| @param[in] coaddWcs Wcs for coadd | |||
| @param[in] visitCatalogs List of loaded source catalogs for each input ccd in | |||
| the coadd, this is used instead of this method loading | |||
| in the catalogs itself | |||
There was a problem hiding this comment.
-> "the coadd. If provided, this is used instead of this method loading in the catalogs itself."
| c = ccdRecord.get(ccdKey) | ||
| dataId = {"visit": int(v), self.config.ccdName: int(c)} | ||
| ccdSources = butler.get("src", dataId=dataId, immediate=True) | ||
| def process_ccd(ccdSources, wcsUpdate): |
Allow PropagateVisitFlags to optionally take a catalog of inputs to work on instead of reading them in itself.
natelust
force-pushed
the
tickets/DM-16873
branch
from
87fde7d
to
9fc727f
Compare
No description provided.