-
Notifications
You must be signed in to change notification settings - Fork 451
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
OffsetUnitCalculusError on String Parsing of degC, degF #386
Comments
I think it is reasonable for there to be an However, I think there's a different issue in parsing offset quantities—you should be able to do |
What if you create the registry with: ureg = UnitRegistry(autoconvert_offset_to_baseunit=True) ? |
Thanks for the suggestion, it works nicely! I probably should've looked at the docs more carefully. However, I still think there should still be a way to allow parsing of |
Why is this closed? |
You can create the unit registry with |
That still doesn't make it more than a workaround, and a horrible one at that:
Modifying global state to achieve a local effect should never pass a code review. And in this case, it would be completely unnecessary: String parsing is well-specified independent of the handling of offset units. |
(As I mentioned in #1342 I think we can use contextvars and contextanagers to modify parsing settings in a safe manner. Take a look at that issue for the proposal) Going back to this specific question, I would suggest that you read this excellent summary #147 by @dalito who also made a PR to make pint safer to handle non multiplicative units. Having said this, current version of Pint has an advantage that might make this open for discussion again. In the older versions of pint |
Indeed, context managers and context variables are the right way to solve the problems mentioned in my first two bullets with changing global state locally. I fully second that. However, I believe it is beside the point: This issue is about parsing strings, which, in pint fails for some of the units (offset units) for no convincing reason. I do not see how I am actually offering a PR in #1349 which fixes this bug - I'm just not sure what the CI is telling me over there. On the larger discussion around what offset units should do or not do, I see that #147 does show a large discussion, which I wasn't aware of. I just recently switched to pint because of pandas support, but have been working with a lot unit packages before (Python: python-quantities and astropy.units, C++: boost.units, nholthaus/units, benri and now mpusz/units, some in JS and Java), and contributed to quite a few of them. Those temperatures / quantity-points / offset-units just always seem to be a challenge. I did note down one observation in #1350 - however, I don't think it is of relevance to the specific issue here. |
Sorry, I might have been a bit ahead of myself. I see that the OP offered a few other attempts on constructing offset quantities, involving a multiplication sign, for which I agree; that is mathematically incorrect and thus correctly raises an Exception. I am specifically referring to the sub-aspect pointed out in one of the early responses and matching the issue title: That |
The point I was trying to make by linking #147 is that this was a conscious decision. It is not a bug. We decided to be less ergonomic when parsing strings to be sure we always do what the user intended ("do not guess" has been always one of our mottos). When the outcome is ambiguous we opt for asking the user to be specific, even if is annoying. An early raised exception is better than a something that gives a wrong/unintended result. With this I am not saying that this cannot be improved. My problem with parsing strings is that associativity is assumed. We expect So before approving a PR, I think we need a concrete, concise and easy to explain ruleset that is surprise free.
Don't be sorry for being enthusiatic. This is most welcome, as well as discussions with fresh eyes. Ps.- Not sure if this is agood idea but in the past I was toying with the idea of making use of things like this: |
I must admit, I haven't read #147 all the way through. However, from what I was able to follow, a significant part of the discussion revolved around products of units (not quantities) involving "offset units". As pointed out in #1350, IMHO such products have no physical meaning and should be disallowed. The bug I see however is that string parsing fails for simple expressions where there is no ambiquity.
I don't think that this is a good idea - I'm sure that to most people However, I do believe that "implicit multiplications" in the context of expressing a quantity are not multiplications at all, but "forming a unit/quantity". This is the approach taken by the PR. I believe it would make sense to make all such "implicit multiplications" higher priority (and right-to-left associativity) than multiplications and divisions. The PR doesn't implement that though. That of course would be a breaking change, because now |
I am a big advocate of the python Zen in general and "refuse to guess" in particular. However, the ambiguity with associativity is not a problem brought about by offset units, but by implicit multiplications. Indeed products of offset units have no physical meaning and thus inherently are ambiguous in what they mean - see #1350. Instead, it is actually the combination of implicit multiplication with division that cause the ambiguity. Both IMHO, the current parser is broken in two ways: a) it throws on unambiguous strings describing "offset quantities" such as My PR fixes a) and leaves b) broken as-is.
If we want to use the opportunity to fix both issues, then, I have three rulesets to offer:
My favourites are 2) as ruleset of |
I encountered the problem by:
>>> from pint import UnitRegistry
>>> ureg = UnitRegistry()
and these following commands
>>> 1*ureg['celsius']
>>> 1*ureg['degC']
>>> 1*ureg.parse_expression('degC')
>>> 1*ureg('degC')
>>> ureg.parse_expression('1*degC')
all will throw error:
using ureg.degC or ureg.degF will not cause the problem
using ureg.Quantity will not cause the problem
My python version
My Pint version
My System version
The text was updated successfully, but these errors were encountered: