python/cpythonPublic

NotificationsYou must be signed in to change notification settings
Fork34.1k
Star71.6k

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

Merged

serhiy-storchaka merged 12 commits intopython:mainfrom

serhiy-storchaka:fractions-from-integer-ratio

Jul 19, 2024

Merged

gh-82017: Support as_integer_ratio() in the Fraction constructor#120271
serhiy-storchaka merged 12 commits intopython:mainfrom
serhiy-storchaka:fractions-from-integer-ratio

Conversation

Copy link

Member

serhiy-storchaka commentedJun 8, 2024•
edited by github-actionsbot
Loading

Any numbers that have the as_integer_ratio() method (e.g. numpy.float128) can now be converted to a fraction.

Issue:Support .as_integer_ratio() in fractions.Fraction #82017

📚 Documentation preview 📚:https://cpython-previews--120271.org.readthedocs.build/

pythongh-82017: Support as_integer_ratio() in the Fraction constructor

62bb662

Any numbers that have the as_integer_ratio() method (e.g. numpy.float128)can now be converted to a fraction.

serhiy-storchaka requested review frommdickinson andrhettinger

June 8, 2024 14:44

bedevere-appbot mentioned this pull request

Jun 8, 2024

Support .as_integer_ratio() in fractions.Fraction#82017

Closed

bedevere-appbot added the awaiting core review label

Jun 8, 2024

rhettinger removed their request for review

June 9, 2024 01:50

Add a versionchanged sirective.

de5b423

picnixz reviewed

Jun 9, 2024

View reviewed changes

Lib/fractions.py Outdated

		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:

raiseTypeError("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).

vstinner approved these changes

Jun 11, 2024

View reviewed changes

Copy link

Member

vstinner left a comment

Choose a reason for hiding this comment

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

LGTM

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

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

MemberAuthor

serhiy-storchakaJul 9, 2024

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.

bedevere-appbot added awaiting merge and removed awaiting core review labels

Jun 11, 2024

skirpichev requested changes

Jul 9, 2024

View reviewed changes

Copy link

Member

skirpichev left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, but we should document somehow that as_integer_ratio() returns components ofnormalized fraction.

Lib/fractions.py Outdated


		elif isinstance(numerator, (float, Decimal)):
		elif (isinstance(numerator, numbers.Number) and
		hasattr(numerator, 'as_integer_ratio')):

Copy link

Member

skirpichevJul 9, 2024

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. Whileas_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 fractionsimport Fractionas F>>>import numbers>>>classR:...def__init__(self,ratio):...self._ratio= ratio...defas_integer_ratio(self):...returnself._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

MemberAuthor

serhiy-storchakaJul 9, 2024

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.

Doc/library/fractions.rstShow resolvedHide resolved

Lib/fractions.py Outdated

		return self

		elif isinstance(numerator, (float, Decimal)):
		elif (isinstance(numerator, numbers.Number) and

Copy link

Member

skirpichevJul 9, 2024

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

MemberAuthor

serhiy-storchakaJul 9, 2024

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 importingdecimal. This is a side benefit of this PR. But adding a fast path for float may be worthwhile.

serhiy-storchakaand others added3 commits

July 9, 2024 13:19

Merge branch 'main' into fractions-from-integer-ratio

adf4869

Update Doc/library/fractions.rst

68bb174

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

Merge remote-tracking branch 'refs/remotes/origin/fractions-from-inte…

4ff41e4

…ger-ratio' into fractions-from-integer-ratio

skirpichev reviewed

Jul 9, 2024

View reviewed changes

Lib/test/test_fractions.py Outdated

		pass
		numbers.Number.register(RatioNumber)

		self.assertEqual((7, 3), _components(F(RatioNumber((7, 3)))))

Copy link

Member

skirpichevJul 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

MemberAuthor

serhiy-storchakaJul 9, 2024

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

Member

skirpichevJul 9, 2024

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.

skirpichev approved these changes

Jul 9, 2024

View reviewed changes

serhiy-storchaka added2 commits

July 9, 2024 13:32

Update an error message.

88bdd20

Add fast path for float.

ab9dc17

mdickinson approved these changes

Jul 14, 2024

View reviewed changes

Copy link

Member

mdickinson left a comment

Choose a reason for hiding this comment

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

LGTM, and +1 for supportingas_integer_ratio in this way.

I'm not totally convinced by the requirement that the number passes anisinstance(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 theisinstance check reduces the risk of this working on non-numeric types that happen to have anas_integer_ratio method that does something entirely unrelated to fractions.

Still, it's an easy change to remove thisisinstance 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).

Copy link

Member

mdickinson commentedJul 14, 2024

I'd welcome thoughts from@rhettinger on this particular change.

Copy link

MemberAuthor

serhiy-storchaka commentedJul 15, 2024

Thenisinstance(numerator, numbers.Number) should be replaced bynot isinstance(numerator, type). We need at least exclude types, because the type has the same method as an instance.