-
Notifications
You must be signed in to change notification settings - Fork 2
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-46414: Add zernikes
astropy table output from CalcZernikesTask
#263
Conversation
848505a
to
7b02e26
Compare
How about other properties that are currently stored in At least a subset of these is very useful, as then we'd have one "all information about each donut pair" table (without the need to utilize multiple tables, especially the I think also throwing in the 4 columns from |
7b02e26
to
a8c2da2
Compare
Yep, I'm currently considering which columns may be the most useful. I think
is probably sufficient for the main table. The table can also have exposure-level metadata for things like
|
Yes, some exposure-level metadata are used to populate an entire donut catalog-length array such as ts_wep/python/lsst/ts/wep/task/cutOutDonutsBase.py Lines 680 to 687 in 3dfe1ab
|
It'd be the same table (same dataset). Just in the metadata, not in the rows.
etc. |
3cdaf0e
to
6c29cf5
Compare
6c29cf5
to
ea1227b
Compare
("extra_sn", "<f4"), | ||
("intra_entropy", "<f4"), | ||
("extra_entropy", "<f4"), | ||
] |
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.
given that sn
and entropy
information is carried over, did you want to also include the decision flags that get created in donutStampSelector
, eg.
SN_SELECT
, ENTROPY_SELECT
, FINAL_SELECT
? Or is it better to keep it separate in donutsIntraQuality
, donutsExtraQuality
?
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 table only adds the selected donuts, so those columns would be trivially True. So I left them out. Keeping the quality tables, (or maybe adding that info to the donutCat tables?) seems reasonable (for a separate ticket).
ea1227b
to
276e0aa
Compare
@@ -6,6 +6,14 @@ | |||
Version History | |||
################## | |||
|
|||
.. _lsst.ts.wep-11.5.0: |
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.
Btw, is the plan to keep outputZernikesRaw
,
outputZernikesAvg
here, and in the next PR remove them? Once that happens, it would have to be a major version change as it would break backwards compatibility. But in the transition, it's fine to have more outputs than less.
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.
Yep, that was my thought as well. It might not be the very next PR where they're removed, but sometime soon. And at that time we'll bump the major version number.
This combines the current outputs from
outputZernikesRaw
andoutputZernikesAvg
into a single new dataset type, usingastropy.table.QTable
as the storage class. Some features:outputZernikesAvg
.Here's a quick preview of the format:
QTables can also hold metadata, so before this gets merged, I'd like to propagate some other items like the camera rotator and boresight pointing. It'd be nice to propagate the flux and blends from donutCat into the rows here too; aggregate everything in one place.