-
Notifications
You must be signed in to change notification settings - Fork 1
DM-53028: Add task to aggregate coadd inputs into a single summary table #162
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
Conversation
526bd29 to
400c522
Compare
jrmullaney
left a comment
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.
Just a couple of doc requests.
|
|
||
| Returns | ||
| ------- | ||
| result : `lsst.pipe.base.Struct` |
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'm worried that "weight" is non-obvious, so I was wondering whether the columns could be listed here, with a description for each. Most would be trivial ("visit": "visit ID"), but then we'd have a description for "weight".
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.
Now you're asking for me to document long standing undocumented things ... and unfortunately this is a place where nobody would look. I'm not sure what to do.
| ) | ||
| coadd_input_summary_tract = lsst.pipe.base.connectionTypes.Output( | ||
| name="coadd_input_summary_tract", | ||
| doc="Summarized coadd inputs (by tract and band).", |
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.
Could you make this more descriptive? e.g., "Table listing the visit and detectors that cover each patch within a tract; also includes weight, good pixel count, band."
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 even know where these show up, but I don't want to put too much in. How about:
doc="Summary table aggregating coaddInputs from multiple coadds (including "
"all coaddInputs tables in a tract for a single band).",
| ------- | ||
| result : `lsst.pipe.base.Struct` | ||
| Result struct containing: | ||
| ``coadd_input_summary`` : `astropy.table.Table` |
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.
See above re: column descriptions.
| ) | ||
| coadd_input_summary = lsst.pipe.base.connectionTypes.Output( | ||
| name="coadd_input_summary", | ||
| doc="Summarized coadd inputs.", |
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 guess this could use the same treatment (see suggestion above).
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.
doc="Summary table aggregating coaddInputs from all coadds, including all "
"tracts and bands.",
| coadd_input_summary_tract["visit"][counter : counter + ndet] = detectors["visit"] | ||
| coadd_input_summary_tract["detector"][counter : counter + ndet] = detectors["ccd"] | ||
| coadd_input_summary_tract["weight"][counter : counter + ndet] = detectors["weight"] | ||
| coadd_input_summary_tract["goodpix"][counter : counter + ndet] = detectors["goodpix"] |
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.
Apologies for not volunteering to review earlier. Is goodpix useful here? The cell-based coadd variant will have everything else but that and it'd be good to keep the output agnostic to the flavor of the coadd.
No description provided.