hgrecco / pint

Operate and manipulate physical quantities in Python
http://pint.readthedocs.org/
Other
2.33k stars 459 forks source link

floordiv (and divmod, modulo, math.floor) raise DimensionalityError #943

Closed westurner closed 1 year ago

westurner commented 4 years ago

floordiv (and divmod, modulo, math.floor) raise DimensionalityError, but I don't understand why because division by a dimensionless integer does not raise DimensionalityError?

import pint
ureg = pint.UnitRegistry()

x = 10 * ureg.inches
n, remainder = divmod(x, 3)

Raises DimensionalityError: Cannot convert from 'dimensionless' (dimensionless) to 'inch' ([length]), which is what the tests say it should raise: https://github.com/hgrecco/pint/blob/4c3114c72bdae1e3c5c9f839659169eeb06071cd/pint/testsuite/test_quantity.py#L715-L720

I could try and redefine floordiv and divmod to just use division and floor? But why is that necessary? That doesn't work either?

import math
def floordiv(x, y):
    n = x / y  # this works just fine
    n_floor = math.floor(n)  # this raises DimensionalityError?
    return n_floor
floordiv(10*ureg.inches, 3)
(10*ureg.inches) % 3 # raises DimensionalityError
hgrecco commented 4 years ago

You are correct. Please submit a PR

westurner commented 4 years ago

Got it. I'm not sure when I will have time for this.

In the meantumey, is there a workaround? I tried dividing by ureg(3) and Quantity(3) but didn't have any luck.

On Sat, Dec 21, 2019, 12:21 AM Hernan Grecco notifications@github.com wrote:

You are correct. Please submit a PR

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/hgrecco/pint/issues/943?email_source=notifications&email_token=AAAMNS3MPWIOZEX5OGRBLSTQZWRXNA5CNFSM4J6AO652YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHOVODQ#issuecomment-568153870, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAMNS6UGQMZMDHQL4NYXOTQZWRXNANCNFSM4J6AO65Q .

hgrecco commented 4 years ago

I am phasing this to 0.11

tecki commented 4 years ago

Good that I came here before the PR got merged....

This issue is blatantly wrong. Floordiv should absolutetly give a DimensionalityError, because it is a dimensionality error. Your example shows this very well. You want to floor-divide 10" by 3. What in world should the result be? Probably you say 3". But this is exactly the dimensionality error that pint tries heavily to prevent you from. Why? Because I guess we agree that 10" = 254 mm. If you floor-divide that by 3 in your logic, you get 84 mm, which is certainly not the 3" you got from before.

You could argue now so what, apparently floor division does not make sense for united values. But also this is not true. It makes a lot of sense: imagine you have a log of 1 m, and want to cut it in 1" pieces. Then you calculate 1 m // 1 " and voilà, you get a nice 39, which is true whatever unit you convert it to, simply because it actually corresponds to a real-world problem.

All the same applies for modulo. In the end, we should fulfill the Python documentation (see https://docs.python.org/3/library/functions.html#divmod).

Let q = a // b:

In any case q * b + a % b is very close to a, if a % b is non-zero it has the same sign as b, and 0 <= abs(a % b) < abs(b).

This quote is also well-defined if there are units involved. And the current implementation does fulfill this rule (hopefully there are no bugs...)

westurner commented 4 years ago

There should be a way to divmod(10*u.inches, 3) and/or divmod(10*u.inches, 3*u.inches).

Could that be done in a context manager that sets e.g. a thread local flag to not raise because the user is explicitly asking to modulo and preserve the unit?

westurner commented 4 years ago

Use case: I have 10ft of 2x4; how many 4ft sections can I make out of that?

Could you show how to drop the unit, perform modulo division, and re-apply the unit?

tecki commented 4 years ago

Your use case is exactly the use case that works (or at least should work, provided there is no bug). divmod(10*u.ft, 4*u.ft) should work seamlessly, simply because it makes sense.

On my machine, latest pint:

>>> divmod(10*u.ft, 4*u.ft)
(<Quantity(2, 'dimensionless')>, <Quantity(2, 'foot')>)

that says: if you have a log of 10 ft length and want to chop 4 ft pieces, you get 2 pieces and are left over with 2 ft of wood. In my example above I even do that with mixed units, like this:

>>> divmod(1*u.m, 1*u.ft)
(<Quantity(3.0, 'dimensionless')>, <Quantity(0.08560000000000012, 'meter')>)

meaning: if you have a log of 1 m length and want to chop it into footlongs, you get 3 foot-long pieces and 85.6 mm left over.

I have no idea what the result of divmod(10*u.inches, 3) should be. Just to make sure, whatever the result is, it should be the same as divmod(254*u.mm, 3)

deeplook commented 2 years ago

What is the status of this issue which I discovered when I was about to open a similar issue, but maybe it better fits as a comment here.

from pint import UnitRegistry

ureg = UnitRegistry()
parse = ureg.parse_expression

# The following should both rather return 1 meter, but return 10 meter.

print(parse("5 m % 2"))
print(parse("(5 % 2) m"))

# The following should both rather return 2 meter, but return 10 meter.

print(parse("5 m // 2"))
print(parse("(5 // 2) m"))

# The following raise a somewhat misleading errors:

# 5 * ureg.m // 2
# DimensionalityError: Cannot convert from 'meter' to 'dimensionless'

# 5 * ureg.m % 2
# DimensionalityError: Cannot convert from 'dimensionless' (dimensionless) to 'meter' ([length])
jules-ch commented 2 years ago

Thanks for the feedback @deeplook.

I can reproduce, it seems binary opertors for floordiv & mod operators are missing in the token evaluator. Evaluation kept going & made implicit multiplication hence the 10 meters result. Might be a good idea to raise Error if token is not mapped to operations.

After adding them we get the proper results.

>>> from pint import UnitRegistry
>>> 
>>> ureg = UnitRegistry()
>>> parse = ureg.parse_expression
>>> parse("5 m % 2")
pint.errors.DimensionalityError: Cannot convert from 'dimensionless' (dimensionless) to 'meter' ([length])
>>> parse("(5 % 2) m")
<Quantity(1, 'meter')>
>>> parse("5 m // 2")
pint.errors.DimensionalityError: Cannot convert from 'meter' to 'dimensionless'
>>> parse("(5 // 2) m")
<Quantity(2, 'meter')>

I'll make a PR for this.

tecki commented 2 years ago

While I am not sure about the missing operators, I can only say that in the example that you give a DimensionalityError is exactly the right thing. What should the result of parse("5 m % 2") be? Just to make sure, it better be the same result as parse("5000 mm % 2"). parse("5 m % 2 m") however should work, simply because it makes sense: if I have a log of 5 m and chop it into 2 m pieces, I have 2 pieces (that's the floordiv) and 1 m left over (that's the modulo).

In short: for floordiv and modulo both operands need to have the same dimensions, like for addition or subtraction, otherwise it makes no sense.

jules-ch commented 2 years ago

It works as expected with the new version. #1471

>>> parse("5 m % (2 m)") 
<Quantity(1, 'meter')>
>>> parse("5 m // (2 m)") 
<Quantity(2, 'dimensionless')>

It only works with the parentheses around 2 m though. Don't know if thats a side effect of the parser.

parse("5 m % 2") should result to Dimensionality error since pint has no idea how to divide your length without unit information.

deeplook commented 2 years ago

While I am not sure about the missing operators, I can only say that in the example that you give a DimensionalityError is exactly the right thing. What should the result of parse("5 m % 2") be? Just to make sure, it better be the same result as parse("5000 mm % 2"). parse("5 m % 2 m") however should work, simply because it makes sense: if I have a log of 5 m and chop it into 2 m pieces, I have 2 pieces (that's the floordiv) and 1 m left over (that's the modulo).

In short: for floordiv and modulo both operands need to have the same dimensions, like for addition or subtraction, otherwise it makes no sense.

When something makes no sense there is the option to define some sense. So one could regard parse("5 m % 2") as equivalent of parse("(5 % 2) m"), but I'm not dogmatic about it.

tecki commented 2 years ago

Well, if you regard it like that, what should the value of parse("5000 mm % 2") be? Given that 5 m and 5000 mm are the same, the result should better be the same.

hgrecco commented 2 years ago

@deeplook I think it is better not to guess the user intentions. We should follow Python's operator precedence link replacing a string by the corresponding quantity object.

deeplook commented 2 years ago

Well, if you regard it like that, what should the value of parse("5000 mm % 2") be? Given that 5 m and 5000 mm are the same, the result should better be the same.

You would stick with the one unit you have, and get a 0 result. Again, I'm not dogmatic on it. Either way it might cause surprises, so let's pick the one closest to the Python conventions.

hgrecco commented 1 year ago

I think this is fixed. Feel free to reopen.

westurner commented 1 year ago

This one is wrong, too, FWIU: https://github.com/hgrecco/pint-pandas/issues/95#issuecomment-1058053030 :

I get an error when using the floordiv method on a PintArray with a scalar, see example below:

from pint_pandas import PintArray
import pandas as pd

p = PintArray([1., 2.], dtype="m")
s = pd.Series(p)

s / 2  # works
s // 2  # pint.errors.DimensionalityError: Cannot convert from 'meter' to 'dimensionless'

floordiv with another PintArray is workin

westurner commented 1 year ago

What could a better Exception message read?

Evidlo commented 1 year ago

I don't agree with some of the conclusions in this thread because they are predicated on the idea that all operations should be unit-aware. Specifically this statement:

This issue is blatantly wrong. Floordiv should absolutetly give a DimensionalityError, because it is a dimensionality error. Your example shows this very well. You want to floor-divide 10" by 3. What in world should the result be? Probably you say 3". But this is exactly the dimensionality error that pint tries heavily to prevent you from. Why? Because I guess we agree that 10" = 254 mm. If you floor-divide that by 3 in your logic, you get 84 mm, which is certainly not the 3" you got from before.

floor(3.333 inch) is an unambiguous operation which translates to e.g. "round this 3.333 length of wood down to the nearest foot". floor as a mathematical concept is unit-agnostic.

If you accept the above argument, then 10 inch // 3 it not a unit-aware operation either and should give us 3 inch.

However, I do agree that 10 inch % 3 should throw a DimensionalityError, because a % b translates to "split this length 'a' piece of wood into pieces of length 'b' and give me what is left over".

tecki commented 1 year ago

Well, if floor is unit-agnostic, as you say, can you please tell me what is floor(3.3 in + 1.1 m)?

Evidlo commented 1 year ago

Well, if floor is unit-agnostic, as you say, can you please tell me what is floor(3.3 in + 1.1 m)?

That doesn't really have anything to do with floor(). The current behavior of Pint when adding compatible units is to use the units from the first summand. So in this case you get floor(3.3 * u.inch + 1.1 * u.m)46 <Unit('inch')>.

tecki commented 1 year ago

Well, if floor is unit-agnostic, as you say, can you please tell me what is floor(3.3 in + 1.1 m)?

That doesn't really have anything to do with floor(). The current behavior of Pint when adding compatible units is to use the units from the first summand. So in this case you get floor(3.3 * u.inch + 1.1 * u.m)46 <Unit('inch')>.

Sure. But for your presumably unit-agnostic mathematical function, what is floor(3.3 in + 1.1 m)? Once we know that, let's let pint behave that way, because pint should reflect - as good as reasonably possible - the underlying mathematical concepts.

deeplook commented 1 year ago

Sure. But for your presumably unit-agnostic mathematical function, what is floor(3.3 in + 1.1 m)? Once we know that, let's let pint behave that way, because pint should reflect - as good as reasonably possible - the underlying mathematical concepts.

Maybe this is more about standardization? After all, units are given by physists, say, with different cultural backgrounds before eventually some standardization emerges, if it does. So one could argue that floor in this case returns the value of the standardized base unit, in this case meters as it is the SI standard. Else you can only argue that it will return the sum of two floor operations in their respective units, which appears a little far-fetched.

That said, I kind of dislike that 3.3 * ureg.inch + 1.1 * ureg.meter returns 46.60708661417323 inch. I really think there should be a preference for international standards, but maybe this opens a whole new issue...

tecki commented 1 year ago

Maybe this is more about standardization? After all, units are given by physists, say, with different cultural backgrounds before eventually some standardization emerges, if it does. So one could argue that floor in this case returns the value of the standardized base unit, in this case meters as it is the SI standard. Else you can only argue that it will return the sum of two floor operations in their respective units, which appears a little far-fetched.

Well, in engineering, the standard base unit is certainly the millimeter. Why not that? And for mass, should the standard base unit be kilogram, or gram? And why? What about the volume, cubic meter, or better liter? Note that floor behaves different for all of these.

Evidlo commented 1 year ago

Sure. But for your presumably unit-agnostic mathematical function, what is floor(3.3 in + 1.1 m)?

The answer is in my comment. floor(3.3 in + 1.1 m) = 46 in

I really think there should be a preference for international standards, but maybe this opens a whole new issue...

I agree that this is a separate issue.

Note that floor behaves different for all of these.

The proposed behavior of floor is the same for all of these because it just preserves the unit of its argument.