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
Correct parsing of CDS units in the presence of multiple divisions #14369
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.
|
@@ -0,0 +1,2 @@ | |||
CDS and MRT tables with units that contain with multiple divisions, such as |
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.
Note that I thought this change was worthwhile specifically mentioning under io.ascii
, since it affects that package most. I have not included a specific test, since I couldn't easily find an existing table that had multiple solidi.
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.
What about that table given in #14355 ?
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'd prefer to use an actually published table... Note that I used the unit that causes the problems in the tests, so the table from #14355 is now fine.
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 don't grok the parsing internals but I trust the tests. I am not sure if you are waiting for original bug report author to review or not, so I am approving but will let you merge when you are ready. Thanks for the quick fix!
@jensmelinder - did you encounter an existing table in which the units had double slashes? If so, I'd be happy to add its |
…ce of multiple divisions
…ce of multiple divisions
…369-on-v5.0.x Backport PR #14369 on branch v5.0.x (Correct parsing of CDS units in the presence of multiple divisions)
…369-on-v5.2.x Backport PR #14369 on branch v5.2.x (Correct parsing of CDS units in the presence of multiple divisions)
Description
This pull request is to address the fact that units with multiple divisions, like "km/s/Mpc", are currently incorrectly parsed in the CDS format (as equivalent to "km Mpc/s" instead of "km/(s.Mpc)").
Fixes #14355