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

custom formatters: pass unit modifiers to the formatter #1448

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

keewis
Copy link
Contributor

@keewis keewis commented Jan 15, 2022

As noted in xarray-contrib/cf-xarray#278, it is important to pass the modifiers to the custom formatters in order to support both short and long formats with the same name if the distinction is not as simple as using the symbol instead of the long name.

This PR shifts the modifier support to the formatters. For the builtin formatters, this is implemented using a helper function which we should probably expose as public API to help with custom formatters that don't need that sort of control.

cc @dcherian

  • Executed pre-commit run --all-files with no errors
  • The change is fully covered by automated unit tests
  • Documented in docs/ as appropriate no custom formatter section yet, requires add a example for a custom unit formatter #1422
  • Added an entry to the CHANGES file

@aulemahal
Copy link
Contributor

As mentioned on the issue, this PR solves #1486. Because of the changes in Units.__format__, it is the Units object that's being passed to format_units and not the UnitsContainer. When "dimensionless", the latter evaluates to False, while the former does not and thus the first lines of format_unit are now irrelevant (or am I wrong?)

(I mean that part :

 if not unit:
        if spec.endswith("%"):
            return ""
        else:
            return "dimensionless"

)

All to say that I would be quite happy with this change in pint. In addition to giving the "~" control to the formatter we use in cf-xarray, it would allow it to print dimensionless units according to the CF-conventions.

@huard
Copy link

huard commented Sep 13, 2022

I was wondering what is the status of this PR? This would be valuable to our team.

@keewis
Copy link
Contributor Author

keewis commented Sep 19, 2022

I've not had the time to finish this so far and there seem to be a few conflicts now, but I remember that @jules-ch had some reservations about delegating the modifiers to the formatters (might have been a misunderstanding, though, that seems to have been a very quick review... see #1450 (comment)).

Edit: @jules-ch, maybe we can continue that discussion?

In any case, I still think this is the correct choice: modifiers are basically options to the formatter, and thus it makes sense to me that the formatter handles them. However, I realize that a lot of formatters would use the default implementation for ~ (and %?), which is why I'd probably make the proposed apply_unit_modifiers helper function public API for custom formatters.

Edit2: for full customization of formatters we should maybe also allow arbitrary modifiers, not just ~, %, and #. However, that will most likely clash with magnitude formatters so we'd need something like #1580.

@keewis keewis changed the title custom formatters: pass the unit modifiers to the formatter custom formatters: pass unit modifiers to the formatter Sep 20, 2022
@Zeitsperre
Copy link

@hgrecco

Hi there, I work alongside @huard and @aulemahal. I've run into some issues today around this. Is there anything we could do to help this along?

@hgrecco
Copy link
Owner

hgrecco commented Jan 23, 2024

@Zeitsperre Take a look at #1913 We are far ahead into change the way the formatter is implemented. We think the proposed solution is superior as it make it easier to fix things and making everything works across different formatters. But also, it allows you to very easy swap the formatter by a new one!

You can actually already test this if you install pint from the master branch. We could use some feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants