gh-82017: Support as_integer_ratio() in the Fraction constructor #120271
Conversation
Any numbers that have the as_integer_ratio() method (e.g. numpy.float128) can now be converted to a fraction.
Lib/fractions.py
Outdated
| @@ -244,7 +243,8 @@ def __new__(cls, numerator=0, denominator=None): | |||
| self._denominator = numerator.denominator | |||
| return self | |||
|
|
|||
| elif isinstance(numerator, (float, Decimal)): | |||
| elif (isinstance(numerator, numbers.Number) and | |||
There was a problem hiding this comment.
I cannot comment on the TypeError below but you should update:
raise TypeError("argument should be a string or a Rational instance")since you do not need a rational only (https://github.com/python/cpython/pull/120271/files#diff-f561eb7eb97f832b2698837f52c2c2cf23bdb0b31c69cf1f6aaa560280993316R281-R282).
Lib/test/test_fractions.py
Outdated
| self.assertRaises(ValueError, F, RatioNumber((7, 3, 1))) | ||
| # only single-argument form | ||
| self.assertRaises(TypeError, F, RatioNumber((3, 7)), 11) | ||
| self.assertRaises(TypeError, F, 2, RatioNumber((-10, 9))) |
There was a problem hiding this comment.
Can you add a test on a fraction which can be simplified, to test "It returns a :class:Fraction instance with exactly the same value." from the doc? For example, the ratio 6/4. (Test that the GCD is not computed to simplify the ratio.)
There was a problem hiding this comment.
No, it is an illegal input. Existing implementations return a simple ratio, simplifying it or checking that it is simplified would be just a waste of time.
Lib/fractions.py
Outdated
| @@ -244,7 +243,8 @@ def __new__(cls, numerator=0, denominator=None): | |||
| self._denominator = numerator.denominator | |||
| return self | |||
|
|
|||
| elif isinstance(numerator, (float, Decimal)): | |||
| elif (isinstance(numerator, numbers.Number) and | |||
| hasattr(numerator, 'as_integer_ratio')): | |||
There was a problem hiding this comment.
Sorry, but this looks wrong for me. While as_integer_ratio() methods for builtin types have ratio in lowest terms and with a positive denominator, we have no such constraint. It's possible to create unnormalized fractions, but some Fraction methods work incorrectly for such input:
>>> from fractions import Fraction as F
>>> import numbers
>>> class R:
... def __init__(self, ratio):
... self._ratio = ratio
... def as_integer_ratio(self):
... return self._ratio
...
>>> numbers.Number.register(R)
<class '__main__.R'>
>>> F(R((6, 4)))
Fraction(6, 4)
>>> F(R((6, 4))) + F(R((2, 2)))
Fraction(5, 2)
>>> 1/F(R((6, 4)))
Fraction(4, 6)
>>> F(R((6, 4))) == F(3, 2)
False
>>> F(R((6, 4))).limit_denominator(3)
Traceback (most recent call last):
File "<python-input-7>", line 1, in <module>
F(R((6, 4))).limit_denominator(3)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^
File "/home/sk/src/cpython/Lib/fractions.py", line 397, in limit_denominator
a = n//d
~^^~
ZeroDivisionError: division by zeroPlease either normalize input or mention requirements for as_integer_ratio() in docs.
There was a problem hiding this comment.
I prefer to update the documentation. For builtin types calling normalization would be a waste of time.
Lib/fractions.py
Outdated
| @@ -244,7 +243,8 @@ def __new__(cls, numerator=0, denominator=None): | |||
| self._denominator = numerator.denominator | |||
| return self | |||
|
|
|||
| elif isinstance(numerator, (float, Decimal)): | |||
| elif (isinstance(numerator, numbers.Number) and | |||
There was a problem hiding this comment.
Performance nitpick. isinstance checks for abc classes are slow:
$ ./python -m timeit -r20 -s 'from numbers import Number as N; import numbers;from decimal import Decimal as D;a=1.1' 'isinstance(a, numbers.Number)'
500000 loops, best of 20: 831 nsec per loop
$ ./python -m timeit -r20 -s 'from numbers import Number as N; import numbers;from decimal import Decimal as D;a=1.1' 'isinstance(a, float)'
5000000 loops, best of 20: 75.3 nsec per loop
In main:
$ ./python -m timeit -r20 -s 'from fractions import Fraction as F;a=1.1' 'F(a)'
50000 loops, best of 20: 6.84 usec per loop
In this branch:
$ ./python -m timeit -r20 -s 'from fractions import Fraction as F;a=1.1' 'F(a)'
50000 loops, best of 20: 8.42 usec per loop
| elif (isinstance(numerator, numbers.Number) and | |
| elif (isinstance(numerator, (float, Decimal, numbers.Number)) and |
There was a problem hiding this comment.
I would like to avoid importing decimal. This is a side benefit of this PR. But adding a fast path for float may be worthwhile.
Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
…ger-ratio' into fractions-from-integer-ratio
Lib/test/test_fractions.py
Outdated
| pass | ||
| numbers.Number.register(RatioNumber) | ||
|
|
||
| self.assertEqual((7, 3), _components(F(RatioNumber((7, 3))))) |
There was a problem hiding this comment.
| self.assertEqual((7, 3), _components(F(RatioNumber((7, 3))))) | |
| # as_integer_ratio() output should be normalized; lets check that | |
| # we just keep this unmodified | |
| self.assertEqual((6, 4), _components(F(RatioNumber((6, 4))))) |
Forgot that. I think, this should address Victor's note.
There was a problem hiding this comment.
This is invalid ratio. The result is undefined.
There was a problem hiding this comment.
This is invalid ratio.
Yeah, but test just checks that we don't touch as_integer_ratio() output.
There was a problem hiding this comment.
LGTM, and +1 for supporting as_integer_ratio in this way.
I'm not totally convinced by the requirement that the number passes an isinstance(number, Number) check. I think I understand the motivation: it's odd to depend on special naming of a method when that name is not a __dunder__ name and it's not explicitly associated with a particular interface, so the isinstance check reduces the risk of this working on non-numeric types that happen to have an as_integer_ratio method that does something entirely unrelated to fractions.
Still, it's an easy change to remove this isinstance check later on if we decide it's not needed (and a much harder change to go the other way and add it later on if it wasn't originally included).
|
I'd welcome thoughts from @rhettinger on this particular change. |
|
Then |
This seems reasonable to me. I would leave off the |
Any numbers that have the as_integer_ratio() method (e.g. numpy.float128) can now be converted to a fraction.
📚 Documentation preview 📚: https://cpython-previews--120271.org.readthedocs.build/