-
Notifications
You must be signed in to change notification settings - Fork 6
DM-53254: Support forcing DRP association outputs to conform to sdm_schemas #426
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
|
@isullivan Please do not add back the |
python/lsst/sdm/schemas/lsstcam.yaml
Outdated
| Default schema for outputs of the LSSTCam Data Release Production Pipeline. | ||
| '@id': '#lsstcamSchema' | ||
| description: 'Default schema for outputs of the LSSTCam Data Release Production Pipeline. | ||
|
|
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.
Remove these extra spaces in the description.
python/lsst/sdm/schemas/lsstcam.yaml
Outdated
| of variable and slowly-moving objects) detected and measured on coadds." | ||
| description: Descriptions of static astronomical objects (or the static aspects | ||
| of variable and slowly-moving objects) detected and measured on coadds. | ||
| '@id': '#Object' |
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.
Remove all @id field additions - they are not needed can be generated automatically (see dp1).
64f0fa9 to
d874984
Compare
d874984 to
da65f10
Compare
| ivoa:ucd: stat.stdev | ||
| - name: u_psfFluxNdata | ||
| datatype: int | ||
| nullable: false |
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.
Looking at the current data in production APDB I see that many records have *_psfFluxNdata fields as NULL. I guess pipelines currently do not set it to 0 when there are no detections in a corresponding band?
Also note that Cassandra does not support NOT NULL columns, but this could be a problem for SQL backend.
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 change came up because we found (in DRP) that these columns were getting set with NaNs and turned into floats. My code will now set any non-nullable columns with values of NaN to 0 instead.
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 we will also need to update existing data to replace NULLs with 0 in those columns? This should be ~trivial for SQL, but not so much for Cassandra. Version number at the top of the file needs to be incremented, please update it to "9.1.1".
ctslater
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.
Looks good from the DRP side, if it's also ok for APDB. I could see leaving mpc_orbits unchanged if you want; since we're not generating that table, nulls aren't going to cause the same problems (and I don't know if their data might have nulls encoded in some way).
da65f10 to
6b2aaa7
Compare
Good point, I'll drop the mpc_orbits changes. |
72deedc to
6282cf5
Compare
ee6e241 to
e0032ce
Compare
Checklist
When making changes to YAML files in the schemas directory: