DM-28092: Document that DiaPipeTask can only handle specific bands #103
Conversation
| datasetRefMap : `NamedKeyDict` | ||
| Mapping from dataset type to a `set` of | ||
| `lsst.daf.butler.DatasetRef` objects | ||
| """ |
There was a problem hiding this comment.
Need a Raises here, since it specifically can raise; maybe a note in there too that it calls the parent and thus can raise whatever the parent raises. I'm not sure what the best way to do that would be: just copying and pasting the parent's Raises isn't ideal, since it could change later.
Also a Returns.
| of the activator. | ||
|
|
||
| This implementation checks to make sure that the filters in the dataset | ||
| are compatible with AP processing as set by the Apdb/DPDD schema. |
There was a problem hiding this comment.
Is Apdb not all-caps for a reason, like DPDD is?
There was a problem hiding this comment.
Because I'm using the name of the task in dax_apdb. Not sure it there is a standard for how to name stuff like so I will go with what the interface object is called.
| dtype=str, | ||
| default=["u", "g", "r", "i", "z", "y"], | ||
| doc="List of bands that are valid for AP processing. To process a " | ||
| "band not on this list, the appropriate, band specific columns " |
There was a problem hiding this comment.
No comma after "appropriate", I think.
| default=["u", "g", "r", "i", "z", "y"], | ||
| doc="List of bands that are valid for AP processing. To process a " | ||
| "band not on this list, the appropriate, band specific columns " | ||
| "must be added to the Apdb schema in dax_apdb.", |
There was a problem hiding this comment.
Apdb all caps nor no caps?
| if ref.dataId["band"] not in self.config.validBands: | ||
| raise ValueError( | ||
| f"Requested '{ref.dataId['band']}' not in validBands list " | ||
| "in DiaPipelineTask config. To process bands not in the " |
There was a problem hiding this comment.
I would just say "not in DiaPipelineConfig.validBands"
| raise ValueError( | ||
| f"Requested '{ref.dataId['band']}' not in validBands list " | ||
| "in DiaPipelineTask config. To process bands not in the " | ||
| "standard Rubin set, ugrizy, you must add the band to the " |
There was a problem hiding this comment.
Parentheses instead of commas around "ugrizy"?
morriscb
force-pushed
the
tickets/DM-28092
branch
from
cf702db
to
f1b601b
Compare
|
Fixed the commit history. I've got a ticket, DM-22743, to implement runQuantum unittests. I can do the unittest for that. |
| validBands = pexConfig.ListField( | ||
| dtype=str, | ||
| default=["u", "g", "r", "i", "z", "y"], | ||
| doc="List of bands that are valid for AP processing. To process a " |
There was a problem hiding this comment.
double space between are valid
| "DiaPipelineConfig.validBands. To process bands not in the " | ||
| "standard Rubin set (ugrizy) you must add the band to the " | ||
| "validBands list in DiaPipelineConfig and add the " | ||
| "appropriate columns to the Apdb schema.") |
There was a problem hiding this comment.
I think this ValueError should include the dataId and/or ref that failed. Otherwise the user might not know exactly where that particular band came from.
There was a problem hiding this comment.
I don't think that would be too helpful. As is it would just report the first dataId it encountered which would possibly just prompt the user to remove them one after the other vs removing them all.
There was a problem hiding this comment.
I'll add a --show-data to the error message.
There was a problem hiding this comment.
Actually, re-reading my error message. I'm fine with keeping it as is.
There was a problem hiding this comment.
One more typo and an addition to the ValueError.
Please squash the commits and cleanup the commit message (a lot of unnecessary "debug X" in there).
Please do add a test for this when you implement runQuantum tests. Maybe add a note to that ticket as a reminder?
Add filter list config. Expand docstring for filter limitations. Fix linting and debug config.
morriscb
force-pushed
the
tickets/DM-28092
branch
from
90b2c52
to
a90a2bd
Compare
No description provided.