8000 gh-82017: Support as_integer_ratio() in the Fraction constructor by serhiy-storchaka · Pull Request #120271 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content
/ cpython Public

gh-82017: Support as_integer_ratio() in the Fraction constructor #120271

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Jul 19, 2024
Merged

gh-82017: Support as_integer_ratio() in the Fraction constructor #120271

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
62bb662
gh-82017: Support as_integer_ratio() in the Fraction constructor
serhiy-storchaka Jun 8, 2024
de5b423
Add a versionchanged sirective.
serhiy-storchaka Jun 9, 2024
adf4869
Merge branch 'main' into fractions-from-integer-ratio
serhiy-storchaka Jul 9, 2024
68bb174
Update Doc/library/fractions.rst
serhiy-storchaka Jul 9, 2024
4ff41e4
Merge remote-tracking branch 'refs/remotes/origin/fractions-from-inte…
serhiy-storchaka Jul 9, 2024
88bdd20
Update an error message.
serhiy-storchaka Jul 9, 2024
ab9dc17
Add fast path for float.
serhiy-storchaka Jul 9, 2024
b0b5444
Merge branch 'main' into fractions-from-integer-ratio
serhiy-storchaka Jul 18, 2024
07bcf81
More ducktyping. Add more tests.
serhiy-storchaka Jul 18, 2024
1539af5
Add an index entry.
serhiy-storchaka Jul 18, 2024
f1c4409
Merge branch 'main' into fractions-from-integer-ratio
serhiy-storchaka Jul 18, 2024
2ace711
Merge branch 'main' into fractions-from-integer-ratio
serhiy-storchaka Jul 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
gh-82017: Support as_integer_ratio() in the Fraction constructor 
Any numbers that have the as_integer_ratio() method (e.g. numpy.float128)
can now be converted to a fraction.
  • Loading branch information
@serhiy-storchaka
serhiy-storchaka committed Jun 8, 2024
commit 62bb662a247f3c1f02d24894afed666e3d47775a
20 changes: 11 additions & 9 deletions Doc/library/fractions.rst
8000
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,24 +18,26 @@ A Fraction instance can be constructed from a pair of integers, from
another rational number, or from a string.

.. class:: Fraction(numerator=0, denominator=1)
Fraction(other_fraction)
Fraction(float)
Fraction(decimal)
Fraction(number)
Fraction(string)

The first version requires that *numerator* and *denominator* are instances
of :class:`numbers.Rational` and returns a new :class:`Fraction` instance
with value ``numerator/denominator``. If *denominator* is ``0``, it
raises a :exc:`ZeroDivisionError`. The second version requires that
*other_fraction* is an instance of :class:`numbers.Rational` and returns a
:class:`Fraction` instance with the same value. The next two versions accept
either a :class:`float` or a :class:`decimal.Decimal` instance, and return a
:class:`Fraction` instance with exactly the same value. Note that due to the
raises a :exc:`ZeroDivisionError`.

The second version requires that
*number* is an instance of :class:`numbers.Rational` or is an instance
of :class:`numbers.Number` and has the :meth:`~as_integer_ratio` method
(this includes :class:`float` and :class:`decimal.Decimal`).
It returns a :class:`Fraction` instance with exactly the same value.
Note that due to the
usual issues with binary floating-point (see :ref:`tut-fp-issues`), the
argument to ``Fraction(1.1)`` is not exactly equal to 11/10, and so
``Fraction(1.1)`` does *not* return ``Fraction(11, 10)`` as one might expect.
(But see the documentation for the :meth:`limit_denominator` method below.)
The last version of the constructor expects a string or unicode instance.

The last version of the constructor expects a string.
The usual form for this instance is::

[sign] numerator ['/' denominator]
Expand Down
7 changes: 7 additions & 0 deletions Doc/whatsnew/3.14.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,13 @@ ast
Added :func:`ast.compare` for comparing two ASTs.
(Contributed by Batuhan Taskaya and Jeremy Hylton in :issue:`15987`.)

fractions
---------

Added support for converting any numbers that have the
:meth:`!as_integer_ratio` method to a :class:`~fractions.Fraction`.
(Contributed by Serhiy Storchaka in :gh:`82017`.)



Optimizations
Expand Down
4 changes: 2 additions & 2 deletions Lib/fractions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

"""Fraction, infinite-precision, rational numbers."""

from decimal import Decimal
import functools
import math
import numbers
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Suggested change
elif (isinstance(numerator, numbers.Number) and
elif (isinstance(numerator, (float, Decimal, numbers.Number)) and

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

hasattr(numerator, 'as_integer_ratio')):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 zero

Please either normalize input or mention requirements for as_integer_ratio() in docs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to update the documentation. For builtin types calling normalization would be a waste of time.

# Exact conversion
self._numerator, self._denominator = numerator.as_integer_ratio()
return self
Expand Down
23 changes: 23 additions & 0 deletions Lib/test/test_fractions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,29 @@ def testInitFromDecimal(self):
self.assertRaises(OverflowError, F, Decimal('inf'))
self.assertRaises(OverflowError, F, Decimal('-inf'))

def testInitFromIntegerRatio(self):
class Ratio:
def __init__(self, ratio):
self._ratio = ratio
def as_integer_ratio(self):
return self._ratio
class RatioNumber(Ratio):
pass
numbers.Number.register(RatioNumber)

self.assertEqual((7, 3), _components(F(RatioNumber((7, 3)))))
Copy link
Contributor
@skirpichev skirpichev Jul 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is invalid ratio. The result is undefined.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is invalid ratio.

Yeah, but test just checks that we don't touch as_integer_ratio() output.

# not a number
self.assertRaises(TypeError, F, Ratio((7, 3)))
# the type also has an "as_integer_ratio" attribute.
self.assertRaises(TypeError, F, RatioNumber)
# bad ratio
self.assertRaises(TypeError, F, RatioNumber(7))
self.assertRaises(ValueError, F, RatioNumber((7,)))
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)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def testFromString(self):
self.assertEqual((5, 1), _components(F("5")))
self.assertEqual((3, 2), _components(F("3/2")))
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2024-06-08-17-41-11.gh-issue-82017.WpSTGi.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Added support for converting any numbers that have the
:meth:`!as_integer_ratio` method to a :class:`~fractions.Fraction`.
0