The Wayback Machine - https://web.archive.org/web/20211027024616/https://github.com/python/cpython/pull/27851
Skip to content
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.

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

bpo-44547: Make Fractions objects instances of typing.SupportsInt #27851

Merged
merged 6 commits into from Oct 21, 2021

Conversation

@mdickinson
Copy link
Member

@mdickinson mdickinson commented Aug 20, 2021

This PR adds an __int__ method to fractions.Fraction, so that an isinstance(some_fraction, typing.SupportsInt) check passes.

https://bugs.python.org/issue44547

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

What if numerator // denominator is an Integral, but not int (for example if numerator and denominator are GMP integers)?

The int constructor implicitly calls index() for the result of __trunc__(), but __int__() should return an exact int.

@mdickinson
Copy link
Member Author

@mdickinson mdickinson commented Aug 20, 2021

What if numerator // denominator is an Integral, but not int

Aargh; good point. I'll fix, and add a test for that case.

@mdickinson
Copy link
Member Author

@mdickinson mdickinson commented Aug 20, 2021

I'll fix, and add a test for that case.

Hmm. The fix is easy; the test less so. The most obvious way to test is to actually create a non-int-subclass that implements numbers.Integral, but that takes quite a lot of code.

@mdickinson mdickinson marked this pull request as draft Aug 20, 2021
@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Aug 22, 2021

I think that we can use an int subclass for simple testing:

class I(int):
    def __floordiv__(self, other):
        return I(int(self) // other)

@mdickinson
Copy link
Member Author

@mdickinson mdickinson commented Aug 22, 2021

I think that we can use an int subclass for simple testing:

Yes, that's probably good enough. Updated.

Lib/fractions.py Outdated
"""trunc(a)"""
"""math.trunc(a)"""
# Note: this differs from __int__ - __int__ must return an int,
# while __trunc__ may return a non-int numbers.Integral object
Copy link
Member

@serhiy-storchaka serhiy-storchaka Aug 22, 2021

I think that __trunc__() can return non-Integral (for example a NumPy array), it will be just incompatible with int() constructor.

Copy link
Member Author

@mdickinson mdickinson Aug 22, 2021

Yes, true. What do you think about just deleting the second line? (Or maybe simply deleting the whole comment.)

Lib/fractions.py Outdated
def __int__(a):
"""int(a)"""
if a._numerator < 0:
return int(-(-a._numerator // a._denominator))
Copy link
Member

@serhiy-storchaka serhiy-storchaka Aug 22, 2021

Would not be better to use operator.index()?

Copy link
Member Author

@mdickinson mdickinson Aug 22, 2021

I'm not sure why; int seems more natural here, given that we're implementing __int__, and it seems a little bit more obvious to me that int must return something of type int (compared to __index__). I think it doesn't really matter all that much - anything implementing Integral and returning different results from __int__ and __index__ is asking for trouble.

Also, in numbers.Integral, __int__ is the more fundamental method: __index__ is implemented in terms of __int__ by default.

@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Aug 22, 2021

What is the performance impact of this change on int()?

@mdickinson
Copy link
Member Author

@mdickinson mdickinson commented Aug 22, 2021

What is the performance impact of this change on int()?

No idea, but I wouldn't expect it to be significant in either direction. I can do some testing, but performance doesn't seem like it ought to be a major concern here.

@mdickinson
Copy link
Member Author

@mdickinson mdickinson commented Aug 22, 2021

No idea, but I wouldn't expect it to be significant in either direction

Hmm; I was wrong. I do see a significant difference in casual timings, apparently arising from the slowness of the 'int' call.

On this branch:

lovelace:cpython mdickinson$ ./python.exe -m timeit -s "from fractions import Fraction; f = Fraction(1001, 65)" "int(f)"
2000000 loops, best of 5: 193 nsec per loop

On the main branch:

lovelace:cpython mdickinson$ ./python.exe -m timeit -s "from fractions import Fraction; f = Fraction(1001, 65)" "int(f)"
2000000 loops, best of 5: 166 nsec per loop

Using operator.index is faster here: if I use

    def __int__(a, _index=operator.index):
        """int(a)"""
        if a._numerator < 0:
            return _index(-(-a._numerator // a._denominator))
        else:
            return _index(a._numerator // a._denominator)

I get something that's consistently faster than either of the above:

lovelace:cpython mdickinson$ ./python.exe -m timeit -s "from fractions import Fraction; f = Fraction(1001, 65)" "int(f)"
2000000 loops, best of 5: 156 nsec per loop

@mdickinson mdickinson marked this pull request as ready for review Aug 22, 2021
@mdickinson mdickinson requested a review from serhiy-storchaka Sep 9, 2021
@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Sep 9, 2021

Just wondering, what is the performance if remove the index call?

@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Sep 9, 2021

Should this change be backported? If no, it should be documented as a new feature (versionchanged and What's New).

@ambv
Copy link
Contributor

@ambv ambv commented Oct 21, 2021

Definitely an enhancement so no backports.

@ambv ambv merged commit d1b2477 into python:main Oct 21, 2021
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants