Issue 32968: Fraction modulo infinity should behave consistently with other numbers (original) (raw)
Created on 2018-02-28 03:10 by elias, last changed 2022-04-11 14:58 by admin. This issue is now closed.
Messages (23)
Author: Elias Zamaria (elias) *
Date: 2018-02-28 03:10
Usually, a positive finite number modulo infinity is itself. But modding a positive fraction by infinity produces nan:
from fractions import Fraction from math import inf 3 % inf 3.0 3.5 % inf 3.5 Fraction('1/3') % inf nan
Likewise, a positive number modulo negative infinity is usually negative infinity, a negative number modulo infinity is usually infinity, and a negative number modulo negative infinity is usually itself, unless the number doing the modding is a fraction, in which case it produces nan.
I think fractions should behave like other numbers in cases like these. I don't think this comes up very often in practical situations, but it is inconsistent behavior that may surprise people.
I looked at the fractions module. It seems like this can be fixed by putting the following lines at the top of the mod method of the Fraction class:
if b == math.inf:
if a >= 0:
return a
else:
return math.inf
elif b == -math.inf:
if a >= 0:
return -math.inf
else:
return aIf that is too verbose, it can also be fixed with these lines, although this is less understandable IMO:
if math.isinf(b):
return a if (a >= 0) == (b > 0) else math.copysign(math.inf, b)I noticed this in Python 3.6.4 on OS X 10.12.6. If anyone wants, I can come up with a patch with some tests.
Author: Mark Dickinson (mark.dickinson) * 
Date: 2018-02-28 16:12
I'm not quite sure why Fraction_instance % float_instance wouldn't simply convert the Fraction to a float and then use the usual floating-point mod. There's the issue that you want to maintain consistency with the floordiv operation, but if Fraction_instance // float_instance also converts its first argument to float, then that consistency should follow automatically.
That's the pattern that's followed for most of the other binary arithmetic operators. It looks like the inconsistency is caused by using _operator_fallbacks for most of the operations, but doing something more ad-hoc for modulo.
I agree that the current behaviour is surprising.
Author: Elias Zamaria (elias) *
Date: 2018-03-01 05:17
Mark, you have some good ideas. A fraction modulo a float is a float, and an integer modulo infinity produces itself as a float, so it seems reasonable that a fraction modulo infinity should be itself converted to a float.
I tried assigning mod and rmod using _operator_fallbacks, like most of the binary operators, and calculations involving infinity behaved as I expected, but one test failed (1.0 % Fraction(1, 10) was no longer 0 because of rounding error).
But then I tried assigning only mod using _operator_fallbacks, and leaving rmod alone, and all the tests passed (including some ones I added to make sure that modulo calculations involving infinity behave like both of us think they should).
As for the floordiv operator, I'm not sure what to think. It would be a bit strange if mod with a float returns a float and floordiv with a float returns an int. It wouldn't be a big deal IMO but it may be easy to change. I tried a simple change to make floordiv with a float to return a float, and changed one test to make sure that it works, and all of the tests passed.
I can make a pull request if anyone wants.
Author: Mark Dickinson (mark.dickinson) *
Date: 2018-03-01 09:50
It would be a bit strange if mod with a float returns a float and floordiv with a float returns an int.
Agreed: if there are floats involved (Fraction // float or float // Fraction), I think the rule should be that we simply fall back to float behaviour, and do whatever float // float does - i.e., return a float. (I'd actually prefer that a mixed Fraction-float operation be a TypeError, but that ship sailed long ago.)
OTOH, if there's a test that's explicitly checking that my_fraction // my_float returns something of type int, then the behaviour is clearly intentional. That means that we should be cautious about changing it, and also means that we probably can't change it in 3.6 or 3.7; it'll have to wait for 3.8. (But I think that's fine; this seems like a minor consistency cleanup that I would expect to affect very few people.)
It looks as though this was a part of PEP 3141 that was never fully implemented: the "Real" type there has a floordiv method that looks like this:
@abstractmethod
def __floordiv__(self, other):
"""The floor() of self/other. Integral."""
raise NotImplementedErrorBut float // float does not currently return an Integral:
>>> import numbers
>>> x = 2.3
>>> isinstance(x, numbers.Real)
True
>>> isinstance(x // x, numbers.Integral)
FalseAnd that definitely shouldn't change: I'd argue against such a change in any case, but backwards compatibility considerations alone mean that we shouldn't change this now to return an integer. Given that, I think it's acceptable to have a mixed fraction-float floor division return a float.
A pull request would be great. Yes, please!
Author: Mark Dickinson (mark.dickinson) *
Date: 2018-03-01 10:01
I'd argue against such a change in any case
And apparently I already did: see issue 22444 for previous discussion on the topic.
Author: Serhiy Storchaka (serhiy.storchaka) *
Date: 2018-03-01 11:03
Not all Fractions can be converted to float.
Fraction(2**2000, 3) // 1.0 Traceback (most recent call last): File "", line 1, in File "/home/serhiy/py/cpython/Lib/fractions.py", line 432, in floordiv return math.floor(a / b) File "/home/serhiy/py/cpython/Lib/fractions.py", line 378, in forward return fallback_operator(float(a), b) File "/home/serhiy/py/cpython/Lib/numbers.py", line 291, in float return self.numerator / self.denominator OverflowError: integer division result too large for a float
What is surprising that the modulo operation can fail even if the end result could be converted to float.
Fraction(22000, 3) % 1 Fraction(1, 3) Fraction(22000, 3) % 1.0 Traceback (most recent call last): File "", line 1, in File "/home/serhiy/py/cpython/Lib/fractions.py", line 440, in mod div = a // b File "/home/serhiy/py/cpython/Lib/fractions.py", line 432, in floordiv return math.floor(a / b) File "/home/serhiy/py/cpython/Lib/fractions.py", line 378, in forward return fallback_operator(float(a), b) File "/home/serhiy/py/cpython/Lib/numbers.py", line 291, in float return self.numerator / self.denominator OverflowError: integer division result too large for a float
Author: Mark Dickinson (mark.dickinson) *
Date: 2018-03-01 18:16
Serhiy: I think both those results are reasonable, and not too surprising. It should be easy to understand that the mixed-type operation converts one of the arguments to float, and if that conversion overflows we get an exception. A similar thing happens when doing something like math.sqrt(2**1024), even though the result is representable.
Author: Elias Zamaria (elias) *
Date: 2018-03-01 22:00
Mark, you said "if there's a test that's explicitly checking that my_fraction // my_float returns something of type int, then the behaviour is clearly intentional". I see exactly that. See https://github.com/python/cpython/blob/bf63e8d55fd2853df3bb99d66de7f428107aadb3/Lib/test/test_fractions.py#L404 In fact, when I said I made floordiv return a float, and "changed one test to make sure that it works", it was that test. So maybe that means we should be more cautious about changing the behavior of floordiv, if we decide to change it.
Author: Elias Zamaria (elias) *
Date: 2018-03-01 22:16
I added the pull request. It includes only my changes to modulo, and not the ones to floordiv.
Author: Mark Dickinson (mark.dickinson) *
Date: 2018-03-02 15:47
[Elias Zamaria]
I see exactly that.
Understood; that's how I interpreted your "changed one test to make sure that it works". It seems we understand each other. :-)
So yes, one always needs to be very cautious about changing deliberate, tested behaviour. But in this case I'm satisfied that I understand what the intent was when that test was written, namely that Real.__floordiv__ returns an Integral. And since that part of PEP 3141 didn't actually get implemented for float // float (and the agreement on #22444 was that the PEP should be changed rather than the behaviour), that intent is now a little bogus, and I think it's reasonable to change the behaviour in this case. The change would make the overall behaviour of mixed-type operations simple to state and understand, and fix a consistency bug. It's also sufficiently much of a corner case that the change seems unlikely to break existing code.
In short, I'm +1 on making your suggested change to floordiv as well as to mod.
I'm adding Jeffrey Yasskin (the original PEP 3141 author) to the nosy list, in case he has any comment. (I don't believe Jeffrey is still watching Python development, but would be happy to be proved wrong.)
Author: Elias Zamaria (elias) *
Date: 2018-03-02 18:32
Any suggestions as to what I should do? I can either update my pull request with my floordiv change, or create a new pull request, or wait a while to see if anyone else has any opinion on changing the behavior.
Author: Mark Dickinson (mark.dickinson) *
Date: 2018-03-05 08:29
Elias: please do go ahead and update your PR.
Author: Elias Zamaria (elias) *
Date: 2018-03-05 17:18
Done.
Author: Mark Dickinson (mark.dickinson) *
Date: 2018-03-14 18:14
Thanks for the PR (and apologies for being slow to look at it). I think we want to use the operator_fallbacks approach for both rfloordiv and floordiv (and similarly for rmod and mod), else we'll get inconsistent results:
Fraction(3) // 5.0 # get float, as expected 0.0 3.0 // Fraction(5) # expect a float, get an integer 0
Please could you make that change and add a couple more tests that cover this case?
Author: Elias Zamaria (elias) *
Date: 2018-03-15 05:38
Mark, what you described (operator_fallbacks for both rfloordiv and floordiv, and for both rmod and mod) was my initial approach. But that broke one test (which floor-divides 1.0 by 1/10 and expects the result to be an integer). I thought about fixing it, to make the behavior more consistent, but I thought that was a bit risky.
Just now, I tried the change again, as you suggested, but I fixed the test to expect a result of 10.0 (a float) instead of 10 (an integer). I got a strange result from that test, saying the result was 9.0.
It seems like this is caused by rounding error, since operator_fallbacks converts both numbers to floats if one of them is a float, and 1/10 can't be represented exactly as a float, so it gets rounded to slightly more than 1/10:
float(Fraction(1, 10)).as_integer_ratio() (3602879701896397, 36028797018963968) Decimal.from_float(float(Fraction(1, 10))) Decimal('0.1000000000000000055511151231257827021181583404541015625')
So yes, I can make that change, but I'm not sure if it would be a good idea. Do you have any thoughts?
Author: Mark Dickinson (mark.dickinson) *
Date: 2018-03-15 09:52
Yes, that sort of thing is going to happen as soon as floating-point enters the mix. There will be surprises from Fraction % float as well as float % Fraction:
from fractions import Fraction Fraction(10**23) // 1e22 9.0
And that's again surprising, because 1e22 is exactly equal to 10**22:
1e22 == 10**22 True
This isn't special to Fractions: the same is true for mixed-type int-float arithmetic:
10**23 // 1e22 9.0
As you say, this is the result of rounding error.
There's not much we can do about that except for make sure that people are aware that precision can be lost when a Fraction is converted to a float. (One could conceivably outlawed mixed-type Fraction-float operations altogether, but rightly or wrongly that's not the decision that was made when the Fraction type was introduced, and changing that now would amount to gratuitous breakage.)
So yes, modifying both floordiv and rfloordiv together (and similarly for mod) is the right thing to do: we definitely don't want Fraction % float and float % Fraction to return different types.
On the testing front, testing the result value of something like 1.0 // Fraction(1, 10) is a little bit dodgy, because the result is only predictable under assumptions that we're using IEEE 754 arithmetic, and that's (at the moment) not an assumption that the Python core makes. I'd suggest using a safer case like 1.0 // Fraction(3, 10).
I thought about fixing it, to make the behavior more consistent, but I thought that was a bit risky.
I think you have excellent instincts here. :-) But in this case, I do think it's better to be consistent, and to have a easy-to-learn and simple-to-remember rule (mixed-type Fraction-float arithmetic operations convert the Fraction to a float, regardless of the operation or the ordering of the operands). The floating-point surprises are a fact of life anyway, regardless of what Fraction does.
Author: Elias Zamaria (elias) *
Date: 2018-03-16 04:12
Mark, I tried Fraction(10**23) // 1e22, and I got 10.
Your 10**23 // 1e22 example was strange, but then int(1e23) and got 99999999999999991611392. That is kind of unexpected but I think it is rare for anyone to do something like that with numbers that big.
I think the fact that floating-point rounding error sometimes causes strange results is not a reason to do really unexpected things like making 1.0 // 1/10 equal 9.0, if that can be reasonably avoided.
I updated my pull request with my change, which you suggested, to make rfloordiv and rmod return a float, but with a small change to _operator_fallbacks to avoid the rounding error, so 1.0 // 1/10 is 10.0. You can see it at https://github.com/python/cpython/pull/5956/commits/1020bb219c1a4fad575ee2309c930ce82a4777fb#diff-14d03bfb59581367725b00781e6f802fL391. What do you think?
Author: Mark Dickinson (mark.dickinson) *
Date: 2018-03-19 18:09
Mark, I tried
Fraction(10**23) // 1e22, and I got 10.
Sorry: that result was with your PR (as it was at the time I wrote that comment). On master, you do indeed get 10.
I think the fact that floating-point rounding error sometimes causes strange results is not a reason to do really unexpected things like making 1.0 // 1/10 equal 9.0, if that can be reasonably avoided.
I understand, but I think in this case, the cure is worse than the disease. I don't think converting the float to a Fraction in mixed-type operations is going to work, for a number of reasons:
- it's changing the behaviour for all the arithmetic operations, not just // and %; that widens the scope for accidental breakage. For example, with the latest commit 038664e6a6288f6113ab96103e57d3b25b39a8c2 on your PR:
Fraction(1) + math.inf inf math.inf + Fraction(1) Traceback (most recent call last): File "", line 1, in File "/Users/mdickinson/Python/cpython/Lib/fractions.py", line 391, in reverse return float(fallback_operator(Fraction(a), b)) File "/Users/mdickinson/Python/cpython/Lib/fractions.py", line 130, in new self._numerator, self._denominator = numerator.as_integer_ratio() OverflowError: cannot convert Infinity to integer ratio
But on master, we have:
import math from fractions import Fraction math.inf + Fraction(1) inf Fraction(1) + math.inf inf
it's counter to the spirit of the numeric tower, where for most arithmetic operations, values are propagated up the tower (from Integral -> Rational -> Real -> Complex) to the first common type before the operation is performed. That keeps things simple; having some cases where values are converted down the tower is going to cause confusion. (Comparisons are necessarily a special case: the need to preserve transitivity of equality and trichotomy means that using the same rules as for arithmetic operations won't fly)
it introduces a non-obvious difference between mixed-mode Fraction-float and int-float operations. For an int x and a float y, I'd expect the result of
x <some_op> yto be identical to the result ofFraction(x) <some_op> y.
So I'm sorry, but I do think having 1.0 // Fraction(1, 10) give 9.0 rather than 10.0 is the least worst option here. It's a surprise, but it's not a new surprise: it's a surprise that's already there in the world of floating-point arithmetic.
1.0 // 0.1 9.0
You're evaluating a discontinuous function at a discontinuity, using a type that's known for its inexactness. In that situation, getting the value for either side of that discontinuity is reasonable.
Author: Elias Zamaria (elias) *
Date: 2018-03-24 03:11
Mark, you have some good points. I didn't fully think about the implications of my change. I undid the change to _operator_fallbacks.
I updated the tests to expect 1.0 // 1/10 to equal 9.0 and 1.0 % 1/10 to equal 0.09999999999999995. That latter number seems a bit awkward though. Can I expect the result to always come out like that, or could it depend on the hardware the test is run on? If we can't depend on that result, do you have any suggestions?
Author: Mark Dickinson (mark.dickinson) *
Date: 2018-08-25 16:51
Sorry that I've taken so long to get back to this. I've just updated the PR, and I think it's ready to go. Looks like it does need you to update your GitHub username here in the "Your Details" section of the bugtracker, though; sorry about that.
If we can't depend on that result, do you have any suggestions?
Yes: check that 1.0 // 1/10 equals 1.0 // 0.1, and similarly for %. I've made that change in the PR.
Author: Elias Zamaria (elias) *
Date: 2018-08-27 03:16
I updated my GitHub username. For the record, it used to be mikez302, and now it is elias6.
Author: Mark Dickinson (mark.dickinson) *
Date: 2018-08-27 06:59
New changeset 393f1ff62e032f20adaed2613a9e2d2d6bcb1eb3 by Mark Dickinson (Elias Zamaria) in branch 'master': bpo-32968: Make modulo and floor division involving Fraction and float consistent with other operations (#5956) https://github.com/python/cpython/commit/393f1ff62e032f20adaed2613a9e2d2d6bcb1eb3
Author: Mark Dickinson (mark.dickinson) *
Date: 2018-08-27 07:13
Fixed for Python 3.8. Thank you!
History
Date
User
Action
Args
2022-04-11 14:58:58
admin
set
github: 77149
2018-08-27 07:13:21
mark.dickinson
set
status: open -> closed
resolution: fixed
messages: +
stage: patch review -> resolved
2018-08-27 06:59:32
mark.dickinson
set
messages: +
2018-08-27 03:16:43
elias
set
messages: +
2018-08-25 17:34:20
mark.dickinson
set
assignee: mark.dickinson
2018-08-25 16:52:07
mark.dickinson
set
versions: - Python 3.6, Python 3.7
2018-08-25 16:51:48
mark.dickinson
set
messages: +
2018-03-24 03:11:42
elias
set
messages: +
2018-03-19 18:09:57
mark.dickinson
set
messages: +
2018-03-16 04:12:13
elias
set
messages: +
2018-03-15 09:52:58
mark.dickinson
set
messages: +
2018-03-15 05:38:37
elias
set
messages: +
2018-03-14 18:14:57
mark.dickinson
set
messages: +
2018-03-05 17🔞35
elias
set
messages: +
2018-03-05 08:29:24
mark.dickinson
set
messages: +
2018-03-02 18:32:50
elias
set
messages: +
2018-03-02 15:47:45
mark.dickinson
set
nosy: + jyasskin
messages: +
2018-03-01 22:16:47
elias
set
messages: +
2018-03-01 22:10:32
elias
set
keywords: + patch
stage: patch review
pull_requests: + <pull%5Frequest5721>
2018-03-01 22:00:13
elias
set
messages: +
2018-03-01 18:16:16
mark.dickinson
set
messages: +
2018-03-01 11:03:33
serhiy.storchaka
set
nosy: + serhiy.storchaka
messages: +
2018-03-01 10:01:22
mark.dickinson
set
messages: +
2018-03-01 09:50:28
mark.dickinson
set
messages: +
2018-03-01 05:17:50
elias
set
messages: +
2018-02-28 16:12:20
mark.dickinson
set
messages: +
2018-02-28 04:24:26
ned.deily
set
nosy: + rhettinger, mark.dickinson
2018-02-28 03:10:51
elias
create