Skip to content

Conversation

@serhiy-storchaka
Copy link
Member

@serhiy-storchakaserhiy-storchaka commented Oct 30, 2019

Copy link
Member

@mdickinsonmdickinson left a comment

Choose a reason for hiding this comment

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

Proposed changes LGTM. For things of exact type float (and possibly also for float subclasses), I think we could dispense with the math_1_to_int complications to get even more speedup. But that doesn't have to happen in this PR.

Do we have tests for the various cases of floats, float subclasses that don't override __floor__, float subclasses that do override __floor__, things that aren't floats but provide a __float__ method, etc.? If not, would it be worth adding such tests?

@mdickinson
Copy link
Member

Do we have tests for the various cases ...

It occurs to me that I don't know how a subclass of float that implements its own __float__ should behave.

>>> class MyFloat(float): ... def __float__(self): ... return 9.0 ... >>> x = MyFloat(16) >>> math.sqrt(x) 4.0 >>> float(x) 9.0 

@serhiy-storchaka
Copy link
MemberAuthor

I think that it would be better to not call __float__ for float subclasses, __int__ and __index__ for int subclasses, __complex__ for complex subclasses, __str__ for str subclasses, etc. We already have a content of corresponding class and can convert it to a C value or to a new Python instance.

@serhiy-storchaka
Copy link
MemberAuthor

Thank you for your review @mdickinson, I addressed your comments and added tests. Could you please take another look?

@serhiy-storchakaserhiy-storchaka merged commit 5fd5cb8 into python:masterNov 16, 2019
@serhiy-storchakaserhiy-storchaka deleted the math-floor-float branch November 16, 2019 16:00
jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
mdickinson pushed a commit that referenced this pull request Feb 9, 2023
`math_1_to_whatever()` is no longer useful, since all existing uses of it convert to `float`. Earlier versions of Python used `math_1_to_whatever` with an integer target; see gh-16991 for the PR where that use was removed.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@serhiy-storchaka@mdickinson@the-knights-who-say-ni@bedevere-bot