GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-82017: Support as_integer_ratio() in the Fraction constructor by serhiy-storchaka · Pull Request #120271 · python/cpython

Conversation

serhiy-storchaka commented Jun 8, 2024 •

edited by github-actions Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          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 from mdickinson and rhettinger

June 8, 2024 14:44

bedevere-app Bot mentioned this pull request

Jun 8, 2024

Support .as_integer_ratio() in fractions.Fraction #82017

Closed

bedevere-app Bot 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

Comment thread

Lib/fractions.py Outdated

		return self

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

picnixz Jun 9, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nineteendo reacted with thumbs up emoji

All reactions

👍 1 reaction

vstinner approved these changes

Jun 11, 2024

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

vstinner Jun 11, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Jul 9, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-app Bot added awaiting merge and removed awaiting core review labels

Jun 11, 2024

skirpichev requested changes

Jul 9, 2024

View reviewed changes

skirpichev left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this 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 of normalized fraction.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/fractions.py Outdated


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

skirpichev Jul 9, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Jul 9, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Doc/library/fractions.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/fractions.py Outdated

		return self

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

skirpichev Jul 9, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Jul 9, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka and others added 3 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 <[email protected]>


          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

Comment thread

Lib/test/test_fractions.py Outdated

		pass
		numbers.Number.register(RatioNumber)

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

skirpichev Jul 9, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Jul 9, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

skirpichev Jul 9, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

skirpichev approved these changes

Jul 9, 2024

View reviewed changes

serhiy-storchaka added 2 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

mdickinson left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this 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 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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mdickinson commented Jul 14, 2024

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka commented Jul 15, 2024

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rhettinger commented Jul 18, 2024

Copy link

Copy Markdown

Contributor

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

This seems reasonable to me.

I would leave off the isinstance() check and use pure duck-typing for the as_integer_ratio() method.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka added 5 commits

July 18, 2024 07:59


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

b0b5444


          More ducktyping. Add more tests.

07bcf81


          Add an index entry.

1539af5


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

f1c4409


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

2ace711

serhiy-storchaka merged commit c8d2630 into python:main

Jul 19, 2024

bedevere-app Bot removed the awaiting merge label

Jul 19, 2024

serhiy-storchaka deleted the fractions-from-integer-ratio branch

July 19, 2024 05:06

scoder added a commit to scoder/quicktions that referenced this pull request

Nov 28, 2024


          Generally use .as_integer_ratio() in the constructor if available.

20d1237

See python/cpython#120271

skirpichev mentioned this pull request

Jun 27, 2026

Fractions instantiation revisited #72902

Open

Reviewers

skirpichev

skirpichev approved these changes

picnixz

picnixz left review comments

vstinner

vstinner approved these changes

+1 more reviewer

mdickinson

mdickinson approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

serhiy-storchaka commented Jun 8, 2024 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skirpichev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skirpichev Jul 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdickinson left a comment

Choose a reason for hiding this comment

Uh oh!

mdickinson commented Jul 14, 2024

Uh oh!

serhiy-storchaka commented Jul 15, 2024

Uh oh!

rhettinger commented Jul 18, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

serhiy-storchaka commented Jun 8, 2024 •

edited by github-actions Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

skirpichev Jul 9, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.