tickets/DM-15104 #211
tickets/DM-15104 #211
Conversation
fred3m
force-pushed
the
tickets/DM-15104
branch
2 times, most recently
from
7ed033a
to
4e4d594
Compare
python/lsst/pipe/tasks/multiBand.py
Outdated
| butler = dataRefList[0].getButler() | ||
| else: | ||
| raise RuntimeError("Neither parsedCmd or args specified") | ||
| return self.TaskClass(config=self.config, log=self.log, butler=butler) |
There was a problem hiding this comment.
Seems like you could just use MergeSourcesRunner instead of writing your own, if you made psfCache a config parameter (which might be a good idea anyway, rather than making it special - though I can see how you might want that, given that it's not an option that should change the outputs).
Even if you do want to keep psfCache as a special command-line argument, I'm hoping you could inherit from MergeSourcesRunner and eliminate most of this code.
There was a problem hiding this comment.
What is psfCache? I just added it because it's something that MeasureMergedCoaddSources had, but I don't necessarily know what it does, and if it's worth keeping.
There was a problem hiding this comment.
Ah, I bet I know, but my knowledge of the details is sufficiently limited that we should ask @PaulPrice.
There was a problem hiding this comment.
It's the lsst::utils::Cache embedded in lsst::afw::detection::Psf that caches the most recent PSF image computations. Changing the capacity of the cache influences the job runtime and memory usage.
python/lsst/pipe/tasks/multiBand.py
Outdated
|
|
||
|
|
||
| class DeblendCoaddSourcesRunner(TaskRunner): | ||
| """Get the psfCache setting into DeblendCoaddSourcesTask""" |
There was a problem hiding this comment.
This doc does not make sense to me, so it probably needs more explanation
python/lsst/pipe/tasks/multiBand.py
Outdated
| """Get the psfCache setting into DeblendCoaddSourcesTask""" | ||
| @staticmethod | ||
| def getTargetList(parsedCmd, **kwargs): | ||
| """! |
There was a problem hiding this comment.
I wont exactly hold something up over this, but I though the policy was that all new docs written would be in numpy doc format
python/lsst/pipe/tasks/multiBand.py
Outdated
| coaddName = Field(dtype=str, default="deep", doc="Name of coadd") | ||
|
|
||
|
|
||
| class DeblendCoaddSourcesRunner(TaskRunner): |
There was a problem hiding this comment.
Really you should probably just use the MergeSourcesRunner task if there isn't any differences between the two. Worst case if you wanted a runner with the new name, just inherit from MergeSourcesRunner with the desired name and pass as the body
python/lsst/pipe/tasks/multiBand.py
Outdated
|
|
||
|
|
||
| class DeblendCoaddSourcesTask(CmdLineTask): | ||
| """DeblendCoaddSourcesTask |
There was a problem hiding this comment.
No real point in having the first line be the same name as the class, as the class name will be displayed in the documentation. Maybe have a short description of class instead
| exposure.getPsf().setCacheCapacity(psfCache) | ||
| sources = self.readSources(patchRef) | ||
| self.singleBandDeblend.run(exposure, sources) | ||
| self.write(patchRef, sources) |
There was a problem hiding this comment.
Looking ahead to changes coming really soon, can you refactor the reading data to its own function that run calls? It should be a trivial change, but will make pipeline task conversion easier.
There was a problem hiding this comment.
I dont know how difficult that will be trying to keep scarlet in mind, if it is too much it is ok, just take a look if you will
There was a problem hiding this comment.
I think that this is a non-trivial change and someone who better understands PipelineTask is better equipped to decided how to split this up, if it can be done in a way that is consistent with other tasks.
python/lsst/pipe/tasks/multiBand.py
Outdated
| dataRef.put(flux_sources, self.config.coaddName + "Coadd_deblendedFlux") | ||
| # Only the multiband deblender has the option to output the | ||
| # template model catalog, which can optionally | ||
| if template_sources is not None: |
python/lsst/pipe/tasks/multiBand.py
Outdated
| if self.config.inputCatalog.startswith("deblended"): | ||
| self.deblended = True | ||
| else: | ||
| self.deblended = False |
There was a problem hiding this comment.
this block can simply be:
self.deblended = self.config.inputCatalog.startswith("deblended")
| @@ -1123,7 +1368,7 @@ def readSources(self, dataRef): | |||
| We also need to add columns to hold the measurements we're about to make | |||
| so we can measure in-place. | |||
| """ | |||
| merged = dataRef.get(self.config.coaddName + "Coadd_mergeDet", immediate=True) | |||
| merged = dataRef.get(self.config.coaddName + self.inputCatalog, immediate=True) | |||
There was a problem hiding this comment.
This will produce different dataset names, is that ok? There is no Coadd_ part of the string in your new version.
There was a problem hiding this comment.
This is expected. Measurements can now be run on one of three different catalogs: the flux conserved deblender output Coadd_deblendedFlux, the scarlet model output Coadd_deblendedModel or the merged catalog (if no deblender is run) Coadd_mergeDet.
| exposure = patchRef.get(self.config.coaddName + "Coadd_calexp", immediate=True) | ||
| filters.append(patchRef.dataId["filter"]) | ||
| exposures.append(exposure) | ||
| # The input sources are the same for all bands, since it is a merged catalog |
There was a problem hiding this comment.
Are you sure this will always be a merged catalog? what happens when it is not one
There was a problem hiding this comment.
This is the same way MeasureMergedCoaddSourcesTask was executed, so the input is always a merged catalog (see https://github.com/lsst/pipe_tasks/blob/tickets/DM-15104/python/lsst/pipe/tasks/multiBand.py#L1019).
python/lsst/pipe/tasks/multiBand.py
Outdated
| self.write(patchRefList[n], fluxCatalogs[n], templateCatalogs[n]) | ||
| filters = [] | ||
| exposures = [] | ||
| for patchRef in patchRefList: |
There was a problem hiding this comment.
see comment on previous commit about splitting out reading
Remove deblending from MeasureMergedCoaddSourcesTask into it's own task, and support the multiband deblender scarlet.
fred3m
force-pushed
the
tickets/DM-15104
branch
from
719ab1e
to
ce962d2
Compare
No description provided.