tickets/DM-16844 #250
tickets/DM-16844 #250
Conversation
| storageClass="SourceCatalog" | ||
| ) | ||
| outputSchema = InitOutputDatasetField( | ||
| doc='Output of the schema used in deblending task', |
There was a problem hiding this comment.
Nitpick: seemingly random switch from double to single quotes here.
| target=SourceDeblendTask, | ||
| doc="Task to deblend an image in one band" | ||
| ) | ||
| exposure = InputDatasetField( |
There was a problem hiding this comment.
Could we call this "coadd" instead of "exposure", so the name reflects the concept rather than the (unfortunately-named) Python/C++ type?
| name='', | ||
| nameTemplate='{}Coadd_deblendedFlux_schema', | ||
| storageClass='SourceCatalog' | ||
| ) |
There was a problem hiding this comment.
You already have a outputSchema (capital 'S') in the base class config; I suspect this (and the one in the multi variant) are not needed.
|
|
||
| class DeblendCoaddSourcesMultiConfig(DeblendCoaddSourcesBaseConfig): | ||
| multiBandDeblend = ConfigurableField( | ||
| target=MultibandDeblendTask, |
There was a problem hiding this comment.
'B' in 'band' should either always be capitalized or never. Looks like the precedent is "always".
There was a problem hiding this comment.
Actually, everything needs to switch to "multiband" as the task name that is imported is "MultibandDeblendTask"
There was a problem hiding this comment.
Yes, but we have multiBand.py here and multiBandDriver.py in pipe_drivers. I'm not actually opposed to switching conventions to Multiband, as I think that's actually more correct given that "multi" isn't a word in its own right, but in that case we should make sure we (eventually) go back and make those consistent with this.
| return self.run(**inputData) | ||
|
|
||
| def _makeSourceCatalog(self, mergedDetections, idFactory): | ||
| # Need to do something more cleaver once we have a real Idfactory here |
There was a problem hiding this comment.
Ok, commenting on a typo in a TODO comment feels really nitpicky, but ... "cleaver".
| return outputTypeDict | ||
|
|
||
| def adaptArgsAndRun(self, inputData, inputDataIds, outputDataIds, butler): | ||
| inputData['filters'] = [dId['abstract_filter'] for dId in inputDataIds['exposures']] |
There was a problem hiding this comment.
Might be good to add a code comment noting that adding a new entry to inputData results in that being passed as a kwarg to run. I suppose that's true for the base class, too, and that gets me wondering how we could improve the code clarity for this pattern without needing to comment it every time we use it, but I do think that for people who aren't you and mean it's not at all clear what's going on. Maybe we should rename the inputData parameter to something like runKwargs?
There was a problem hiding this comment.
I made a note on my to-do list to come back to this later, as discussed in person
natelust
force-pushed
the
tickets/DM-16844
branch
from
db28b20
to
14294f0
Compare
The stack has two different ways of deblending, one band at a time, or all bands together. Because these have different units of work in a quantum graph, the work that was once done by a single command line task has now been split up into two different PipelineTasks. Of note is that these tasks are only used when processing in a PipelineTask sense, and the original DeblendCoaddSources task must still be used when processing with command line tasks.
natelust
force-pushed
the
tickets/DM-16844
branch
from
14294f0
to
6cf5cc0
Compare
Introduce PipelineTasks for doing deblending
The stack has two different ways of deblending, one band at a time,
or all bands together. Because these have different units of work
in a quantum graph, the work that was once done by a single
command line task has now been split up into two different
PipelineTasks. Of note is that these tasks are only used when processing
in a PipelineTask sense, and the original DeblendCoaddSources task must
still be used when processing with command line tasks.