-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 "inline" option for unicode and console unit formats to give negative powers instead of fractions #14393
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.
|
Thanks! Is this related to #14386 ? |
Would need a change log and maybe a what's new too. |
Failures are definitely related, so those also need to be addressed. |
It is not related, Maybe a better solution would be to add an |
dc6df64
to
72438dc
Compare
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.
In principle, all for this! If implemented via subclasses, though, I think it is best to just override relevant methods rather than use an attribute.
That said, I'm actually not sure it is most sensible to add subclasses -- to me, these are very much stylistic changes for which keyword arguments or configuration items are more appropriate -- a bit like np.printoptions
.
We've gone already a bit in that direction for Quantity.to_string
, which has a subfmt
option. Should Unit.to_string()
have something similar, which gets passed on to the format classes?
I worry in part because of seeing the test cases: why does 'generic' collect its negative units in parentheses and uses a solidus, while 'console' and 'unicode' use negative powers. That difference suggests there should be an option to choose, but do we really want an extra subclass for that too? Adding it as options to format.to_string()
seems more logical...
@olebole - I made a separate issue about the formatting -- see #14398. My sense is that the easiest is for the format to default to inline -- which would likely be easier to implement if it is a keyword argument... p.s. Sorry for making what looked like a simple PR complicated... But it would be nice to get it more or less right... |
I am answering in the main thread since it may make the discussion easier. Overwriting Having an argument in If one would add an argument to @classmethod
def to_string(cls, unit, inline=False):
[…] how would this be propagated to the user, f.e. in Another (more complicated) way could be to convert the formatters from classes to instances - this way one could have one unicode and one unicode_inline instance, and easily create others if needed (f.e. with central dots instead of spaces between the factors) -- however probably all use cases here could be better handled with subformats. Can you point me to how quantities handle/specify subformats? |
@olebole -- interesting! Just on the practical questions: arguments could be passed down from Lines 748 to 759 in f489872
I agree it would not help astropy/astropy/units/quantity.py Lines 1393 to 1511 in f489872
|
On the subclasses vs |
For
|
I'll have to think a little more, in particular about the implications for |
72438dc
to
3d602e6
Compare
I now again stepped back a bit and implemented just the basics; an "inline" option in I'd propose to take this as an acceptable intermediate step and to merge this. It only minimally changes the public API and postpones any "quantity format minilanguage", allowing to think here more about a more holistic approach. It also resolves somehoe #14398, because of the switch to inline formatting as default. |
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! This works well, and I agree we might as well break it up into small steps. Only real comment is about a method I think is no longer used, plus a small suggestion.
With the "inline" option for the formats, positive/negative is no longer a good description of the parts. While they are formed from parts of the unit with positive and negative power if not inline, their use is nominator and denominator in a fraction.
3d602e6
to
6f9822e
Compare
6f9822e
to
6d40752
Compare
pre-commit.ci autofix |
f47279b
to
19df748
Compare
19df748
to
dd3312a
Compare
As the files were touched here anyway, I deleted/replaced the single |
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 changing those too -- definitely clearer! Since tests now all passed, I'll merge. Nice!
Description
This PR adds inline versions of the unicode and console format representations for units. My primary interest is
unicode_inline
, but since I modified the console format as well,console_inline
came out for (almost) free -- it is up to discussion whether it is useful enough to keep.It looks like this:
The change is done in the same way as in #2622 (which addedlatex_inline
), to keep the same structure.Im am also unsure what the use real case for the existing non-inline versions is as they are broken when used for quantities (IMO they should at least be disabled for formatting units, or switched to the proposed inline variants then):