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-38629: implement floor and ceil for float #16985

Merged

vstinner merged 3 commits into python:master from isidentical:bpo-38629

Dec 15, 2019

Contributor

isidentical commented Oct 29, 2019 •

edited by bedevere-bot

https://bugs.python.org/issue38629

the-knights-who-say-ni added the CLA signed label

bedevere-bot added the awaiting review label

vstinner reviewed

Member

vstinner left a comment

What is the performance impact on math.floor(1.0) and math.ceil(1.0)? Faster, slower, no significant impact?

Objects/floatobject.c Outdated Show resolved Hide resolved

mdickinson reviewed

Objects/floatobject.c Outdated Show resolved Hide resolved

Member

mdickinson commented Oct 29, 2019

This PR needs tests.

mdickinson requested changes

Member

mdickinson left a comment •

edited

Thanks for this PR. Some changes are needed:

The PR needs unit tests.
The implementations won't do the right thing on machines with 32-bit longs: there's no adjustment for non-integer values in the non-fast path.
As @vstinner suggested, it would be much simpler to just use the libm functions ceil and floor.
There's no real need for the fast path, given #15611: I'd suggest dropping that and simply sending the output of ceil and floor into PyLong_FromDouble.

bedevere-bot removed the awaiting review label

bedevere-bot commented Oct 29, 2019

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

bedevere-bot added the awaiting changes label


        bpo-38629: implement __floor__ and __ceil__ for float

0b36750

isidentical force-pushed the isidentical:bpo-38629 branch from e4ff4b8 to 0b36750

Contributor Author

isidentical commented Oct 30, 2019

I have made the requested changes; please review again

bedevere-bot added awaiting change review and removed awaiting changes labels

bedevere-bot commented Oct 30, 2019

Thanks for making the requested changes!

@mdickinson: please review the changes made to this pull request.

bedevere-bot requested a review from mdickinson


        update generated files

e1ac18c

Contributor Author

isidentical commented Oct 30, 2019

I submitted benchmarks to bpo

brandtbucher added the type-enhancement label

mdickinson approved these changes

Member

mdickinson left a comment

Changes LGTM; thank you! The performance is still a concern, but if some form of #16991 gets merged then that wouldn't be so much of an issue.

@serhiy-storchaka @vstinner Does the current state of the PR seem okay to you?

bedevere-bot added awaiting merge and removed awaiting change review labels

serhiy-storchaka reviewed

Objects/floatobject.c Outdated Show resolved Hide resolved

Member

serhiy-storchaka commented Oct 30, 2019

#16991 will solve the performance issue for exact floats, but not for subclasses.

Member

mdickinson commented Oct 31, 2019

will solve the performance issue for exact floats, but not for subclasses.

I don't think there's too much reason to care about fine-tuning of performance for float subclasses. The most common subclass is NumPy's float64, and if you're using NumPy you're likely using NumPy's floor instead of math.floor anyway.

isidentical requested a review from serhiy-storchaka

isidentical requested a review from vstinner


        use PyFloat_AS_DOUBLE for getting direct value without checking

40db4db

isidentical force-pushed the isidentical:bpo-38629 branch from b2bd4eb to 40db4db

Member

vstinner commented Dec 5, 2019

me:

What is the performance impact on math.floor(1.0) and math.ceil(1.0)? Faster, slower, no significant impact?

I checked the current implementation of math.floor(): there is a fast-path for exact type float which does something like PyLong_FromDouble(floor(PyFloat_AS_DOUBLE(number))).

I'm fine with calling the __floor__() method for subclasses. That's the purpose of subclasses: being able to override some methods. That's part of the Python semantics.

Member

vstinner commented Dec 5, 2019

I checked the current implementation of math.floor(): there is a fast-path for exact type float which does something like PyLong_FromDouble(floor(PyFloat_AS_DOUBLE(number))).

Oh, that's @serhiy-storchaka 's PR #16991 which has just been merged. I didn't notice ;-)

vstinner approved these changes

Member

vstinner left a comment

LGTM. There is no performance overhead on math.floor() and math.ceil() for exact float.

I'm fine with having a small overhead for float subclasses. I didn't measure the overhead, I expect it to be really small or even not significant.

Member

vstinner commented Dec 5, 2019

@mdickinson @serhiy-storchaka: I'm not sure that you agree, so I didn't merge the PR. I approve the PR, so I'm fine with merging it. What about you?

Member

vstinner commented Dec 14, 2019

@mdickinson @serhiy-storchaka: I plan to merge this PR next Friday (December 21) if no one replies.

@isidentical: Please ping me next Friday if I forget to merge your PR.

Member

mdickinson commented Dec 15, 2019

@vstinner Merging is fine with me.

vstinner merged commit cb8b946 into python:master

4 checks passed

4 checks passed

Azure Pipelines PR #20191203.29 succeeded

Details

bedevere/issue-number Issue number 38629 found

Details

bedevere/news News entry found in Misc/NEWS.d

continuous-integration/travis-ci/pr The Travis CI build passed

Details

bedevere-bot removed the awaiting merge label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

You can’t perform that action at this time.