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
bug: add missing Sun in VOUnits simple units #15832
Conversation
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
7c1f5e6
to
5a3a0d0
Compare
Thanks! Can you please add a test? |
Sure! But I cannot find any existing tests on the simple units. What about something that asserts that they can be created without warnings but that adding a prefix raises a warning? |
Something is better than nothing. I would recommend a test that could prevent your astroquery bug from creeping back in. Thanks! |
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.
Thanks for the addition! Would indeed be good to have a test, could be as simple as adding the following to tests/test_format.py::test_vounit_details
after the test for "pix":
# Regression test for astropy/astroquery#2480
assert u.Unit("Sun", format="vounit") is u.Sun
EDIT: I actually think this is implicitly tested in the overall tests of VOUnits, but I agree with @pllim that we might as well be explicit for anything that caused a problem!
also check that a warning is raised when prefixing a simple unit
Thanks for the suggestion, done :) |
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.
Nice! thanks, @ManonMarchand!
…832-on-v6.0.x Backport PR #15832 on branch v6.0.x (bug: add missing Sun in VOUnits simple units)
Hi astropy!
Description
The unit
Sun
is missing from the VOUnitssimple_units
list. This is documented in table 5 of the standard (version 1.1, from 2023/12/15) :(and I checked with one on the authors of the standard)
This PR just adds it. This should solve astropy/astroquery#2480