New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-30022: Create input/output pipeline mimicking LSSTDESC/dia_pipe AssociationTask #519
Conversation
9446eda
to
703afbe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mostly looks good. It is straightforward to understand what is supposed to happen. I have a few comments, mostly about variable names and documentation.
"fakesType": ""}): | ||
diaSourceTables = pipeBase.connectionTypes.Input( | ||
doc="Catalog of calibrated DiaSources.", | ||
name="{fakesType}{coaddName}Diff_diaSrcTable", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put 'f' before the string literal here and in "assocDiaSourceTable" and "diaDiaObjectTable".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've looked through some other Gen3 tasks in pipe_tasks and none of them use f
strings in these places. Pretty sure this is a specific functionality of the PipelineTask framework to fill these at run time.
storageClass="DataFrame", | ||
dimensions=("tract", "patch"), | ||
) | ||
diaDiaObjectTable = pipeBase.connectionTypes.Output( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what is being conveyed by repeating 'dia' in the variable name.
For the documentation, I think this means "Catalog of DiaObjects that have been created by associating DiaSources", but this is a little hard to parse with the current language. Can you clarify the doc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo.
"""Trim DiaSources to the current Patch and run association. | ||
|
||
Takes in the set of DiaSource catalogs that covers the current patch, | ||
trims to them to the dimensions of the patch, and [TODO: eventually] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"trims to them" -> "trims them"
defaultTemplates={"coaddName": "deep", | ||
"warpTypeSuffix": "", | ||
"fakesType": ""}): | ||
diaSourceTables = pipeBase.connectionTypes.Input( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this one table or multiple tables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are potentially multiple tables. Hence the plural. I've updated the comment here for clarity.
for idx, row in cat.iterrows(): | ||
sphPoint = geom.SpherePoint(row["ra"], row["decl"], geom.degrees) | ||
inPatch = innerPatchBox.contains(wcs.skyToPixel(sphPoint)) | ||
yield inPatch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far, the output of _trimToPatchGen
is always immediately converted to a list. Do you anticipate a different use? If not, for clarity I would just have the function return an array, but that's a personal preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair, I wrote this when there were multiple checks going on at once. I can switch it to an array return.
import lsst.geom as geom | ||
import lsst.pex.config as pexConfig | ||
import lsst.pipe.base as pipeBase | ||
from .coaddBase import makeSkyInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is a strict rule, but I think this line should be separated and below the import lsst...
lines .
Bounding box of the patch. | ||
wcs : `lsst.geom.SkyWcs` | ||
Wcs of the tract. | ||
skyMap : `` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skyMap isn't an input parameter.
|
||
Yields | ||
------ | ||
isInTractPatch : `bool` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable name below is inPatch
.
diaSourceTables = pipeBase.connectionTypes.Input( | ||
doc="Catalog of calibrated DiaSources.", | ||
name="{fakesType}{coaddName}Diff_diaSrcTable", | ||
storageClass="DataFrame", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below, do you need to specify that this is a pandas.DataFrame?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Storage class here is a specific Gen3 butler wrapper for parquet/pandas.
280f3a9
to
ec0b63f
Compare
a6f2dd0
to
73c8ec2
Compare
Add imports and fix linting. Debug remaining simple pipeline. Add position cuts to run method. Finish up initial task. Switch to generator. Debug iterations. Add unittest. Additionally add docstrings. Debug unittest. Update docs. Finish up doc strings. Rename pipeline. Remove tract test. Add output for flattened DiaSource table.
Debug pipeline and unittest. Update run method docstring. Fix linting.
Fix up docs. Change output and name to _trimToPatch. Fix name of diaObjectTable in output struct. Fix linting. Fix typo. Fix typo in DRP yaml output name. Remove merge text.
73c8ec2
to
0b8c844
Compare
No description provided.