-
Notifications
You must be signed in to change notification settings - Fork 6
DM-52666: Add guider columns to visit1_quicklook #404
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
| This includes observatory data from the FITS image header metadata and measurements and metrics from Rapid Analysis and other data processing. | ||
| version: | ||
| current: "1.4.0" | ||
| current: "1.4.1" |
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.
Adding columns should be a minor version bump to 1.5.0, according to the CDB docs.
| datatype: float | ||
| description: Rotator drift over exposure in arcsec. | ||
| ivoa:unit: arcsec | ||
| - name: guider_rotator_standard_deviation |
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.
We should double-check with Aaron if the rotator name is appropriate here. This quantity is the theta angle of the focal plane, arctan(y/x).
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 looks fine from data engineering. It should still get an additional approval from someone on the ConsDB team before being merged. (@bbrondel ?)
I only noticed that the ivoa:ucd definitions are not included consistently, but this is not required and hasn't been handled uniformly for CDB schemas up to now.
|
PRs for ConsDB schemas should not be merged until they have been fully deployed including TAP Schema so that the Schema Browser never promises something that isn't actually there. |
For changes to be visible in the TAP service, they would need to be included into a weekly release and so merged into Merging sdm_schemas PRs has no effect on what is deployed to the USDF/RSP via Phalanx, which has to be updated separately, but you're right that it updates the schema browser. And so mismatches between the schema browser and what is actually deployed to the USDF/RSP could occur. I'm not sure what is the best approach to avoid this aside from merging on Wednesdays so that the changes get included in the weekly and then deploying that weekly tag in the next day's patch window. (If you have other ideas, please let me know. I know this is kind of an ongoing concern affecting all CDB PRs.) |
bed3241 to
22eaaa4
Compare
| - name: guider_magnitude_drift | ||
| "@id": "#visit1_quicklook.guider_magnitude_drift" | ||
| datatype: float | ||
| description: Magnitude drift over exposure in arcsec. |
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.
| description: Magnitude drift over exposure in arcsec. | |
| description: Magnitude drift over exposure in magnitudes. |
| ivoa:ucd: time.interval | ||
| ivoa:unit: s | ||
| - name: guider_roi | ||
| "@id": "#visit1_quicklook.guider_roi_size" |
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.
Please use a consistent name and ID.
| "@id": "#visit1_quicklook.guider_t_mean" | ||
| datatype: float | ||
| description: Mean second moment T for all measurements over the exposure. | ||
| ivoa:unit: arcsec |
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 the correct unit? Or should it be arcsec2?
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.
You are correct!
| "@id": "#visit1_quicklook.guider_e2_mean" | ||
| datatype: float | ||
| description: Mean second moment e2 for all measurements over the exposure. | ||
| ivoa:unit: arcsec |
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 the unit correct on e1 and e2, or are they dimensionless?
3b96ce6 to
ecda624
Compare
ecda624 to
bd27beb
Compare
796e022 to
bd27beb
Compare
bd27beb to
e52ec58
Compare
Checklist
When making changes to YAML files in the schemas directory: