meas_base review for DM-1945: tasks for multi-band coadd processing #19
Conversation
| @@ -102,7 +105,7 @@ def fetchReferences(self, dataRef, exposure): | |||
| def _makeArgumentParser(cls): | |||
| parser = lsst.pipe.base.ArgumentParser(name=cls._DefaultName) | |||
| parser.add_id_argument("--id", "deepCoadd_forced_src", help="data ID, with raw CCD keys + tract", | |||
| ContainerClass=lsst.coadd.utils.CoaddDataIdContainer) | |||
| ContainerClass=lsst.coadd.utils.ExistingCoaddDataIdContainer) | |||
There was a problem hiding this comment.
I understand at least part of why @PaulPrice started using this (it makes it easier to deal with patches where there are no data), but I'm not totally sold on whether silently skipping data IDs where coaddition failed is the right logic; it seems like that could lead to problems being discovered later than they ought to be.
@laurenam, do you know if this change is necessary (rather than just a convenience) for the rest of this issue? If not, I'd be inclined to suggest we take it off this issue and RFC it before we bring it over. If so, I think we bring it over now but open an issue for reconsidering it in the future.
There was a problem hiding this comment.
I think it's fine to leave it as just CoaddDataIdContainer (I had asked you guys about this earlier and you leaned the other way, but I'm happy to revert!)
There was a problem hiding this comment.
Heh, I guess I must react to changes differently when my I have my reviewer hat on.
There was a problem hiding this comment.
@TallJimbo , did you want to me revert to CoaddDataIdContainer in multiBand.py as well (you didn't comment on it there, so I just want to make sure this was a global comment on this functionality).
There was a problem hiding this comment.
Yes, it was intended as a global comment (but if you didn't interpret it that way, that's fine too - I see you've already merged).
There was a problem hiding this comment.
Indeed I did go with the global interpretation, so the merge pipe_tasks has been reverted. I did merge the addition of the class to coadd_utils, so if/when we every want to use it, it's there.
When these Tasks are run in their own stand-alone scripts, the schema is loaded through the butler. However, when they are run as part of a common operation, the Tasks are instantiated before the schemas are written out, and so they need to be given the schemas at construction time. This change makes both possible: the schema is used if provided, otherwise loaded from the butler. Note: this is essentially a cherry-pick from HSC-1063 except that we still refer to Coadd_src_schema instead of Coadd_ref_schema. This will likely change once we have the multiband processing working on the LSST stack.
While we may want to load from deepCoadd_src, under the multiband processing scheme it's deepCoadd_ref, so that suffix needs to be customisable. Have NOT gone to deepCoadd_ref exclusively yet, so the old forced photometry should still work.
Since there's now no calibration in ProcessCoaddTask, there's no deepCoadd_calexp dataset. But the deepCoadd is already calibrated (using the inputs) and has everything we need, so we load that instead. Conflicts: python/lsst/pipe/tasks/forcedPhotCoadd.py
laurenam
force-pushed
the
u/lauren/DM-1945
branch
from
67e3279
to
062aff4
Compare
No description provided.