DM-31924: update to new system for catalog ID generation #168
Conversation
There was a problem hiding this comment.
I've wondered about our modification of inputs in runQuantum in all these Tasks, instead of generating the idFactory inside run itself. Maybe that will make more sense when I review the pipe_tasks ticket, but I wonder whether we could clean that up if we're needing to modify the API here anyway.
| @@ -187,7 +189,7 @@ def run(self, | |||
| Filter band of the science image. | |||
| ccdVisitId : `int` | |||
| Identifier for this detector+visit. | |||
| funcs : `lsst.pipe.tasks.functors.Functors` | |||
| funcs : `lsst.pipe.tasks.functors.Functors`, optional | |||
| Functors to apply to the catalog's columns. | |||
There was a problem hiding this comment.
Missing the new idGenerator docstring.
There was a problem hiding this comment.
I think I must have ultimately intended to remove the idGenerator argument instead, since it isn't actually used in run and I didn't mark ccdVisitId as deprecated. I think that's because it uses just the ccdVisitId (instead of e.g. also making an lsst.afw.table.IdFactory) and I was going for minimal changes. If you'd prefer for me to deprecate ccdVisitId and pass an idGenerator instance for better consistency with other tasks, I can do that instead.
There was a problem hiding this comment.
Good question! I think since we don't need the packer itself and this operates per-detector/visit, we can keep it as-is. Also, all that's used for is a log message and setting the value in the table.
There was a problem hiding this comment.
I wonder whether we could clean that up if we're needing to modify the API here anyway.
I don't see one or the other as clearly better here, so I'm inclined to just leave things as they are. Passing the data ID is nice in that it moves more logic inside run, but passing the IdGenerator lets it do some abstracting over the "is there a data ID" question, avoiding what may be a lot of if data_id is None checks in run.
| @@ -187,7 +189,7 @@ def run(self, | |||
| Filter band of the science image. | |||
| ccdVisitId : `int` | |||
| Identifier for this detector+visit. | |||
| funcs : `lsst.pipe.tasks.functors.Functors` | |||
| funcs : `lsst.pipe.tasks.functors.Functors`, optional | |||
| Functors to apply to the catalog's columns. | |||
There was a problem hiding this comment.
I think I must have ultimately intended to remove the idGenerator argument instead, since it isn't actually used in run and I didn't mark ccdVisitId as deprecated. I think that's because it uses just the ccdVisitId (instead of e.g. also making an lsst.afw.table.IdFactory) and I was going for minimal changes. If you'd prefer for me to deprecate ccdVisitId and pass an idGenerator instance for better consistency with other tasks, I can do that instead.
| @@ -187,7 +188,7 @@ def run(self, | |||
| Filter band of the science image. | |||
| ccdVisitId : `int` | |||
| Identifier for this detector+visit. | |||
| funcs : `lsst.pipe.tasks.functors.Functors` | |||
| funcs : `lsst.pipe.tasks.functors.Functors`, optional | |||
There was a problem hiding this comment.
I think we can just remove this funcs entirely: it's entirely unused: the code uses self.funcs, set in __init__.
This was a bug, but a very minor one a the IDs in the forced-phot catalogs aren't really used and probably don't need to exist. But as long as they do it's best to make these IDs embed the data IDs like all other kinds of source IDs (including diffim forced source IDs a few lines above this change).
This deprecation isn't strictly necessary to move away from code being deprecated elsewhere on this ticket, but it makes this task more consistent with all of the others
This was apparently unused. If we need something like this back, we should pass a CatalogIdGenerator instance instead for consistency with other tasks.
This was being ignored in favor of self.funcs already.
TallJimbo
force-pushed
the
tickets/DM-31924
branch
from
ae1f6dd
to
177ee94
Compare
No description provided.