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

pint 0.18 does not allow to explicitly override "~" modifier in default_format #1407

Closed
cpascual opened this issue Nov 4, 2021 · 7 comments · Fixed by #1419
Closed

pint 0.18 does not allow to explicitly override "~" modifier in default_format #1407

cpascual opened this issue Nov 4, 2021 · 7 comments · Fixed by #1419

Comments

@cpascual
Copy link
Contributor

cpascual commented Nov 4, 2021

Hi, I see a change in the behavior from pint 0.17 to 0.18 which I think it might be a bug.

Consider the following code:

from pint import UnitRegistry

ureg = UnitRegistry()
q = ureg.Quantity("1m")

print(f"implicit (before): {q}")
print(f"explicit (before): {q:d}")

ureg.default_format = "~"

print(f"implicit (after):  {q}")
print(f"explicit (after):  {q:d}")

With pint <=0.17 I get:

implicit (before): 1 meter
explicit (before): 1 meter
implicit (after):  1 m
explicit (after):  1 meter

With pint 0.18 I get:

implicit (before): 1 meter
explicit (before): 1 meter
implicit (after):  1 m
explicit (after):  1 m

IMHO, the behaviour of 0.17 is the expected one, since the default_format should not affect at all when a explicit format is used.

@cpascual cpascual changed the title pint 0.18 does allow to explicitly override "~" modifier in default_format pint 0.18 does not allow to explicitly override "~" modifier in default_format Nov 4, 2021
@keewis
Copy link
Contributor

keewis commented Nov 4, 2021

I was thinking that this is a bug until I remembered that #1371 introduced the concept of "magnitude" and "unit" formatters (it was used before, but not clearly separated). This means that the format string of f"{q:d}" basically is split into ("d", ""), where the Unit format is empty and thus the default from default_format is used (which is also split into magnitude and unit format). If your format was f"{q:dP}" instead, you'd get the expected result (i.e. no ~)

I agree that that's surprising, at least given the current documentation, and I don't think I got any feedback regarding this yet (to be fair, it's pretty well hidden in the code and I forgot to raise it in the PR).

@cpascual
Copy link
Contributor Author

cpascual commented Nov 4, 2021

Thanks @keewis for the quick answer!

Just one thing: you mention using f"{q:dP}", which indeed gives the same output in both 0.17 and 0.18, but it uses the pretty format (this becomes evident if we redefine, e.g., q = ureg.Quantity("1m**2")) which may not be desired.

In order to use the default format one can use f"{q:dD}" , but only in pint>=0.18 (unit format "D" is not supported in pint<=0.17)

Therefore, I agree that it is not a bug, but IMHO it should be listed in the CHANGES file as a "Breaking Change" of 0.18

@keewis
Copy link
Contributor

keewis commented Nov 4, 2021

yes, that's a breaking change that should probably have gone through a deprecation cycle. @hgrecco, @jules-ch, do you have a opinion on this? Should we revert that change in behavior and add a deprecation cycle?

@hgrecco
Copy link
Owner

hgrecco commented Nov 5, 2021

I think Pint 0.18 is the correct one, but I do agree this was a breaking change. I would agree on releasing a 0.18.1 in the next days rolling back this and deploying this change in 0.19 What I am not clear exactly what exactly you will revert in the code? Is not this very intertwined with the new formatting code? Would reversion cause a very ugly code?

@keewis
Copy link
Contributor

keewis commented Nov 5, 2021

Would reversion cause a very ugly code?

not at all, we'd need to change three lines:

pint/pint/quantity.py

Lines 397 to 398 in 3df6536

uspec = extract_custom_flags(spec)
ustr = format(obj.units, uspec)
would become

    ustr = format(obj.units, spec) 

and

spec = spec or extract_custom_flags(self.default_format)

would become

        spec = extract_custom_flags(spec or self.default_format)

(plus the code to issue a DeprecationWarning / FutureWarning?)

It would probably also be good to add an explanation to the formatting docs (if it's not there already?)

@jules-ch
Copy link
Collaborator

jules-ch commented Nov 5, 2021

Sounds good to me, we can issue a new release soon with #1403 too.

+1 on the docs.

@hgrecco
Copy link
Owner

hgrecco commented Nov 5, 2021

Let me know when the PRs are ready and I can make the release

jkotan pushed a commit to desy-fsec/taurus that referenced this issue Jun 23, 2023
pint 0.18 introduced a change that breaks some of our unit tests.
See: hgrecco/pint#1407
Adapt the tests to work both with pint <=0.18
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 a pull request may close this issue.

4 participants