Skip to content
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-42003: Fix UCD errors in SDM schemas #170

Merged
merged 2 commits into from Dec 12, 2023
Merged

Conversation

JeremyMcCormick
Copy link
Collaborator

@JeremyMcCormick JeremyMcCormick commented Dec 4, 2023

This fixes a single typo in obsloctap and three UCDs not following standard syntax in imsim.

The imsim fixes were:

  • pos.eq.decErr -> stat.error;pos.eq.dec
  • pos.eq.raErr -> stat.error;pos.eq.ra
  • pos.eq.ra_dec_Cov -> stat.covariance;pos.eq.ra;pos.eq.dec

These were checked for correctness with the ucd.parse_ucd function in astropy via Pydantic command line validation.

Copy link
Collaborator

@gpdf gpdf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a suggestion for how to deal with the covariance. I am open to pushback on this if there is any prior art out there.

yml/imsim.yaml Show resolved Hide resolved
yml/imsim.yaml Show resolved Hide resolved
@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-42003 branch 2 times, most recently from 73bd9be to 7468a27 Compare December 11, 2023 22:14
This corrects the misspelling of 'ime:duration' to 'time:duration'.
This corrects the syntax of UCDs that included invalid words according
to the UCD1+ controlled vocabulary. The corrections were straighforward
aside from changing 'pos.eq.ra_dec_Cov' to
'stat.covariance;pos.eq.ra;pos.eq.dec'. This correction indicates that
the column represents the complex covariance between the two positional
measurements, which are listed explicitly with two different words.
@JeremyMcCormick
Copy link
Collaborator Author

I've made a suggestion for how to deal with the covariance. I am open to pushback on this if there is any prior art out there.

I requested a re-review. I believe we decided to go with my initial change of listing both positional measurements separately rather than just using 'pos.eq'.

Copy link
Collaborator

@gpdf gpdf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good now. Yes, I agree, we converged on stat.covariance;pos.eq.ra;pos.eq.dec in preference to stat.covariance;pos.eq after consultation with IVOA people.

@JeremyMcCormick JeremyMcCormick merged commit 419bf45 into main Dec 12, 2023
4 checks passed
@JeremyMcCormick JeremyMcCormick deleted the tickets/DM-42003 branch December 12, 2023 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants