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

Allow fraction and inline options for unit.to_string() #14449

Merged
merged 8 commits into from Feb 28, 2023

Conversation

mhvk
Copy link
Contributor

@mhvk mhvk commented Feb 23, 2023

This pull request continues the work started by @olebole in #14393 in giving the user options for how to represent a unit in a given format. It deviates from #14393 in using fraction as the primary option argument, to indicate whether or not bases raised to negative powers should be shown as is (fraction=False) or a fraction should be used (fraction=True).

In addition, an inline option allows the choice between using a solidus (inline=True) or a displayed fraction (inline=False). The latter is only available for latex, console, and unicode formats.
EDIT: I changed to follow option 1 below and only have a fraction={False|True|'inline'|'display'} argument.

Opening as a draft since there are some implementation questions:

  1. Does the nomenclature make sense? An alternative that I considered was having only a fraction option, with possible values False, 'inline' and 'display', with True as a short-cut to the default fraction style 'inline'. This is more concise and perhaps better (I'm wavering!);
  2. Right now, the inline option is ignored for formats that do not support it. Should it raise an error instead? EDIT: yes, since machine-readable formats should be strict about what they allow, and raise if an option would make the format no longer readable.

To do:

  • Raise on bad options.

p.s. Just in case: the inline option was just introduced and is only in -dev - hence the replacement of the #14393 changelog fragment with a new one (EDIT: and the skip-changelog-checks entry since that is misled by the deletion of 14393.feature.rst).
p.s.2 For my own convenience, on top of #14437; will rebase once that is in.

@mhvk mhvk added units Affects-dev PRs and issues that do not impact an existing Astropy release Enhancement labels Feb 23, 2023
@mhvk mhvk added this to the v5.3 milestone Feb 23, 2023
@github-actions
Copy link

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.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@github-actions
Copy link

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

@mhvk mhvk added the skip-changelog-checks Tells bot to skip changlog checks label Feb 23, 2023
@mhvk mhvk force-pushed the units-format-fraction-and-inline-options branch from 65b8eb0 to 9c97e24 Compare February 23, 2023 01:20
Copy link
Member

@nstarman nstarman left a comment

Choose a reason for hiding this comment

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

Another nice improvement. I like seeing methods exchanged for super().
Also, is this the right venue to change to tuple? Sorry to keep banging on this drum, but this is one of many many ways we can speed up the unit internals.

astropy/units/core.py Outdated Show resolved Hide resolved
@eerovaher
Copy link
Member

We should consider implementing these extra options as components in unit format names, i.e. to_string(format="console_fraction") instead of to_string(format="console", fraction=True). The two reasons I am proposing this are:

  1. it would be more consistent with the already existing "latex" and "latex_inline" unit formats
  2. it would allow using the new options through the Python string format specifier mechanism, i.e. it would also be possible to use f"{q:console_fraction}".

@mhvk
Copy link
Contributor Author

mhvk commented Feb 23, 2023

@eerovaher - @olebole and I discussed this in his initial PR, which introduced initial, and ended up concluding that the opposite was better, both to keep Unit.to_string() easy to use, and to avoid a proliferation of format classes.

I also prefer not to tie our hands too much in how we use Quantity.__format__ - we'd want the "mini-language" to be a more or less logical extension of what python already has, yet also keep freedom to support, e.g., sexagesimal in Angle, etc. Of course, if we do end up wanting to use underscores in format names to indicate whether or not to have a fraction, etc., it is easy enough to do a .split('_') and just interpret. Or still create those subformats and override .to_string() in each to set the defaults (or perhaps change things a little so that the defaults are class attributes).

@mhvk mhvk force-pushed the units-format-fraction-and-inline-options branch from 9c97e24 to 5d9b61f Compare February 23, 2023 22:27
@mhvk
Copy link
Contributor Author

mhvk commented Feb 23, 2023

OK, encouraged by @nstarman's sense, I tried having only a fraction={False|True|'inline'|'display'} option and it feels better. It is just the last commit, so one can look at that to check whether it is indeed preferred over bool fraction and inline (with latter only used if fraction=True).

Obviously, if we agree on either direction, I'll squash/rebase appropriately.

@mhvk mhvk marked this pull request as ready for review February 23, 2023 22:31
docs/whatsnew/5.3.rst Outdated Show resolved Hide resolved
@mhvk mhvk force-pushed the units-format-fraction-and-inline-options branch from 5d9b61f to f1517fc Compare February 24, 2023 01:44
docs/whatsnew/5.3.rst Outdated Show resolved Hide resolved
@eerovaher
Copy link
Member

I think it's important that the already existing "latex" and "latex_inline" unit format pair would not become an exception to whatever mechanism ends up being adopted for the other formats. Splitting the unit format name on underscores seems to me like a good way to implement this new and useful functionality for the other formats while maintaining consistency with the design choices already made and avoiding complicating the code too much.

Another thing to consider, beyond the scope of this pull request but very much related to the idea of consistency between unit formats, is that because for "unicode" and "console" the inline version is clearly preferable over the multiline fraction then perhaps "latex" should be made to behave like "latex_inline" too.

@mhvk
Copy link
Contributor Author

mhvk commented Feb 24, 2023

@eerovaher - I agree that we should be careful before changing how we do quantity formatting (unit formatting is rarely useful on its own). For the regular user, all we do here is to ensure that quantities formatted as console or unicode are sensible, and hopefully at the same time make the output more logically controllable. (But for future quantity formatting, to me most useful would be to get Angle right first (i.e., sexagesimal)...)

On inline for latex by default: right now, quantities use the equivalent of fraction='inline', while for a unit on its own, it is fraction='display'. If we want to make it the same as console and unicode, it would change to fraction=False for both cases, which I think is indeed fine (disregarding the note at

# NOTE: This should change to display format in a future release
).

But for this PR, probably best to stick to the premise that it does not change things for the two most common cases, of using an notebook (and thus _repr_latex_) or a terminal (and thus regular __repr__).

astropy/units/format/base.py Show resolved Hide resolved
docs/whatsnew/5.3.rst Outdated Show resolved Hide resolved
@mhvk
Copy link
Contributor Author

mhvk commented Feb 26, 2023

OK, with all your help, I think this PR to introduce the fraction option for unit.to_string() is now ready. I quite like it!

Copy link
Member

@olebole olebole left a comment

Choose a reason for hiding this comment

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

That is great work! I like it as well!

Copy link
Member

@nstarman nstarman left a comment

Choose a reason for hiding this comment

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

Overall this is a large improvement. My comments are minor and mostly about adding more docstring. Feel free to ignore the code speed bumps.

astropy/units/core.py Outdated Show resolved Hide resolved
- `False` : display unit bases with negative powers as they are;
- 'inline' or `True` : use a single-line fraction;
- 'multiline' : use a multiline fraction (available for the
``latex``, ``console`` and ``unicode`` formats only).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
``latex``, ``console`` and ``unicode`` formats only).
``"latex"``, ``"console"`` and ``"unicode"`` formats only).

Also, are these available as astropy.units.format.Base objects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, should be strings. The formats are documented at https://docs.astropy.org/en/latest/units/index.html#module-astropy.units.format, but the class docstring are not really useful as is -- clearly, only used indirectly... Anyway, I'll leave them as strings for now!

astropy/units/core.py Show resolved Hide resolved
astropy/units/format/base.py Show resolved Hide resolved
Comment on lines +316 to +323
parts = []
if m not in ("", "1"):
parts.append(m)
if ex:
if not ex.startswith("-"):
ex = "+" + ex
parts.append(f"10{cls._format_superscript(ex)}")
return cls._times.join(parts)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
parts = []
if m not in ("", "1"):
parts.append(m)
if ex:
if not ex.startswith("-"):
ex = "+" + ex
parts.append(f"10{cls._format_superscript(ex)}")
return cls._times.join(parts)
if ex and not ex.startswith("-"):
ex = "+" + ex
return cls._times.join(
((m,) if m in ("", "1") else ())
+ ((f"10{cls._format_superscript(ex)}",) if ex else ())
)

Should be a mild speed bump as it reduces intermediates and extending lists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this makes it quite a bit less readable, and not obviously faster given the creation and addition of tuples... Since it is not in a fast path anyway, I prefer to just stick with what was there...

Copy link
Member

Choose a reason for hiding this comment

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

SGTM.

astropy/units/format/cds.py Show resolved Hide resolved
@mhvk mhvk force-pushed the units-format-fraction-and-inline-options branch from 909ccaf to 242b236 Compare February 27, 2023 21:44
Copy link
Member

@nstarman nstarman left a comment

Choose a reason for hiding this comment

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

Thanks @mhvk!

@mhvk mhvk merged commit ba7f072 into astropy:main Feb 28, 2023
@mhvk mhvk deleted the units-format-fraction-and-inline-options branch February 28, 2023 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Affects-dev PRs and issues that do not impact an existing Astropy release Enhancement skip-changelog-checks Tells bot to skip changlog checks units
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants