Repository navigation
Refactor units formats FITS and VOUNIT a bit more - #14437
Conversation
This prepares it to be able to use in-line fractions as well.
|
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.
|
|
👋 Thank you for your draft pull request! Do you know that you can use |
|
If the sorting isn't necessary, can we switch to tuples, re the last PR. |
|
a bit more... ? |
|
A bit more than #14416 😺 |
|
I added a commit that does not strictly belong, but I'm lazy: in the previous PRs, starting with #14393, we called the numerator of a fraction the nominator, which is incorrect (although apparently so common that Fraction.nominator exists and that none of the many reviewers noticed it...). Anyway, the last commit corrects it. |
|
Well, actually |
olebole
left a comment
olebole
left a comment
There was a problem hiding this comment.
Other than the non-blocking question about formats for FITS and VOUNIT, I son't have further issues.
| return super().format_exponential_notation(val, format_spec) | ||
|
|
||
| @classmethod | ||
| def to_string(cls, unit, inline=True): |
There was a problem hiding this comment.
Should machine readable output formats have options like inline here?
There was a problem hiding this comment.
Indeed, you are right, one can never have a "non-inline" format for FITS and VOUNIT. In a way, this is kind of a placeholder for a change to fraction in #14449 - I think FITS at least allows in-line fractions.
Since the same holds for cds where currently there is no check either (since it just inherits), how about we keep it here but ensure that in #14449 anything that is not allowed by the standard raises an error?
|
Just to make sure I'm getting this right. While this PR adds "inline" arguments in a few places, the long-term plan is to remove that argument in favor of the "fraction" argument? The "fraction" argument will be checked for correct input and e.g. error in VOUNIT if given a non-inline option. |
This pull request follows up on #14416 to refactor also FITS and VOUNIT a bit more. Mostly, it was an oversight of not realizing that the sorting done in those formats was not actually necessary, as it just duplicated what is done already in their superclass,
generic.Note that on purpose this does not change the sorting -- which I think should be done but which changes a lot of tests, so needs separate discussion! (EDIT: see #14449)
EDIT: I added a commit that does not strictly belong, but I'm lazy: in the previous PRs, starting with #14393, we called the numerator of a fraction the nominator, which is incorrect (although apparently so common that
Fraction.nominatorexists and thatFraction.nominatordoes not exists; apparently, that line inutils.pyis never exercised...