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 string parsing of offset units #1349

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

Conversation

burnpanck
Copy link

The fix implements special handling for implicit multiplications in the string parser when the left operand is not a quantity:
Then, it creates a quantity directly, instead of multiplying the two operands.
For this to work, the string_preprocessor had to be modified not to replace implicit multiplications with explicit multiplications.

@burnpanck
Copy link
Author

This fix will probably not work for compound offset units such as degC m. However, I believe that these things are neither fully supported by the rest of pint, nor should they be; see #1350.

@burnpanck
Copy link
Author

Not completely sure what I see in the CI results here (haven't worked with GitHub CI recently).
Locally on my machine, a number of tests would fail already in the master branch where I branched this PR off.

@dalito
Copy link
Contributor

dalito commented Jul 15, 2021

Would e.g. ureg.Quantity("42 degC/m") raise an error with your PR? In other words, is the idea to allow parsing of strings with an offset unit only if there is no other unit present in the string and if the unit´s exponent is 1? I think with such limitations parsing of strings with offset units would be Ok.

@burnpanck
Copy link
Author

burnpanck commented Jul 16, 2021

I believe 42 degC/m would indeed fail. All this PR does is treating <number> <unit> combinations not as a multiplication, but as "forming of a quantity" (notice the absence of a * - multiplications remain untouched). I believe that is what a human does when reading such expressions. I haven't touched the grouping priority or associativity of operations, so all product units will remain to be parsed from left to right, such that this "forming of quantity" only happens between the number and the first unit, followed by multiplications by further units.

Generally: As I tried to stipulate in #1350, products involving offset units are generally questionable, as they lack a physical meaning - IHMO they should be completely disallowed. However, this PR simply follows the rules set by pint; it makes the expression parser accept "forming of quantity" involving offset units, and then continues evaluating the expression given in the string according to pint's rules. I think that is the least surprising behaviour.

@burnpanck
Copy link
Author

It seems that #386 is still unsolved. At our company, we have been using a fork of pint for the past two years to get faster access to some fixes that we care for particularly. One of these things is the ability to serialise quantities to json or yaml and back elegantly as a single, human-readable string rather than an ugly [magnitude, units] construct. We are currently updating all our dependencies, and I was hoping to see this fixed now.

@hgrecco, @dalito: What was speaking against merging this two years ago? Obviously, by now, this PR has become stale, but I would be willing to revive it if there is any chance that it would be merged.

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.

OffsetUnitCalculusError on String Parsing of degC, degF
3 participants