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

Add radial velocities table #72

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

arjunsavel
Copy link
Collaborator

Addresses #69.

Follows the same procedure as adding the parallax table. add the quantity, its error, some validators, some schema tests.

Tried to correspond to the ingest script: astrodbtoolkit/astrodb_utils#52

@arjunsavel arjunsavel requested a review from kelle August 14, 2024 15:26
schema/schema_template.py Outdated Show resolved Hide resolved
({"radial_velocity_km_s": None}, ValueError),
({"radial_velocity_error_km_s": None}, None),
({"radial_velocity_error_km_s": 30}, None),
({"radial_velocity_error_km_s": -30}, None),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the uncertainty be allowed to be negative?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I guess not! I was thinking about the "negative parallaxes are not unexpected" Gaia results, and I must've mixed them up with errors. I can fix this for parallaxes and RVs.

schema/schema_template.py Outdated Show resolved Hide resolved
Co-authored-by: Kelle Cruz <kellecruz@gmail.com>
@arjunsavel arjunsavel changed the title Add radial velicities table Add radial velocities table Oct 22, 2024
@kelle
Copy link
Contributor

kelle commented Oct 22, 2024

add test_radial_velocities.py to test the content.

@arjunsavel
Copy link
Collaborator Author

add a module for testing this table — at min check the counts in the table

@kelle
Copy link
Contributor

kelle commented Dec 10, 2024

The table is being added in #86 . But we'll still need the tests from this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants