DM-15310: {{Refactor ForcedPhotImageTask.runDataRef to new run.}} #127
Conversation
| @@ -121,17 +121,14 @@ def __init__(self, butler=None, refSchema=None, **kwds): | |||
| self.makeSubtask('catalogCalculation', schema=self.measurement.schema) | |||
|
|
|||
| def runDataRef(self, dataRef, psfCache=None): | |||
| """!Measure a single exposure using forced detection for a reference catalog. | |||
| """!Organize a single exposure for forced detection for a reference catalog. | |||
There was a problem hiding this comment.
I suggest reverting this change. This method does measure -- though it calls run to do the job. Consider appending "using a dataRef" to the old brief description, instead, to differentiate it from the brief description for run.
| @@ -157,9 +171,10 @@ def runDataRef(self, dataRef, psfCache=None): | |||
| ) | |||
| self.catalogCalculation.run(measCat) | |||
|
|
|||
| self.writeOutput(dataRef, measCat) | |||
| return(measCat) | |||
There was a problem hiding this comment.
In Python return is a keyword, not a function, so return measCat. That said, run should almost always return an lsst.pipe.base.Struct (for future expansion without breaking existing code) so I suggest:
return lsst.pipe.base.Struct(measCat=measCat)
and updating the @return documentation accordingly.
| @param[in] refWcs The WCS for the references. | ||
| @param[in] dataRef An lsst.daf.persistence.ButlerDataRef. Needed for method calls and logging. | ||
|
|
||
| @param[out] measCat Reference sources are generated with generateMeasCat() in the measurement |
There was a problem hiding this comment.
This is a returned value instead of an argument that is modified, so use @return instead of @param[out].
Also I find the description confusing. I assume measCat is measured sources, not reference sources, but the description starts with "Reference sources". Consider something like "Measured sources" or "Measured forced sources" or "Sources measured by generateMeasCat"
|
|
||
| def makeIdFactory(self, dataRef): | ||
|
|
There was a problem hiding this comment.
I suggest undoing this change (i.e. remove the blank line you inserted)
There was a problem hiding this comment.
As per @yalsayyad I think some change are needed, or perhaps just giving up on the refactor
| @@ -143,6 +140,23 @@ def runDataRef(self, dataRef, psfCache=None): | |||
| if psfCache is not None: | |||
| exposure.getPsf().setCacheSize(psfCache) | |||
| refCat = self.fetchReferences(dataRef, exposure) | |||
|
|
|||
| measCat = self.run(exposure, refCat, refWcs, dataRef) | |||
There was a problem hiding this comment.
This is a struct containing measCat, so this code looks wrong. I think you want something like this:
runResult = self.run(...)
self.writeOutput(dataRef, runResult.measCat)
|
|
||
| self.writeOutput(dataRef, measCat) | ||
|
|
||
| def run(self, exposure, refCat, refWcs, dataRef): |
There was a problem hiding this comment.
As @yalsayyad pointed out on the ticket, please don't pass the dataRef to run.
If self.attachFootprints really needs a dataRef (I haven't looked through the stack to see how that is used) then I don't see an obvious alternative and it may be best to abandon this refactoring.
If self.attachFootprints doesn't actually need the dataRef (but can use something less butler-ish as a replacement) then the rest of the info can be passed into run as arguments: an id factory, an exposure ID and the dataId (the latter as an optional argument, since it's just used for logging).
|
I've updated the branch to include changes to remove the dataRef entirely from the run() method. |
| @param[in] exposureId Optional unique exposureId used for random seed in measurement task. | ||
|
|
||
| @return result An lsst.pipe.base.Struct containing the catalog of forced measurement results | ||
| from measurement.run(). |
There was a problem hiding this comment.
Please name the field(s) here, e.g.
An lsst.pipe.base.Struct containing:
- measCat: a catalog of forced measurement results (lsst.afw.table.SourceCatalog)
Small refactor of ForcedPhotImageTask to add a run method. ForcedPhotCcd and ForcedPhotCoadd will both inherit this method.