FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-38629: implement __floor__ and __ceil__ for float by isidentical · Pull Request #16985 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

bpo-38629: implement __floor__ and __ceil__ for float - #16985

Merged
vstinner merged 3 commits into
python:masterfrom
isidentical:bpo-38629
Dec 15, 2019
ConversationCommits (3)ChecksFiles changed
Merged

bpo-38629: implement __floor__ and __ceil__ for float#16985
vstinner merged 3 commits into
python:masterfrom
isidentical:bpo-38629

Conversation

isidentical commented Oct 29, 2019 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

vstinner reviewed Oct 29, 2019
View reviewed changes

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread Objects/floatobject.c Outdated
mdickinson reviewed Oct 29, 2019
View reviewed changes
Comment thread Objects/floatobject.c Outdated

mdickinson commented Oct 29, 2019

Copy link
Copy Markdown
Member

This PR needs tests.

mdickinson suggested changes Oct 29, 2019
View reviewed changes

mdickinson left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Oct 29, 2019

Copy link
Copy Markdown

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.

isidentical commented Oct 30, 2019

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again

bedevere-bot commented Oct 30, 2019

Copy link
Copy Markdown

Thanks for making the requested changes!

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

isidentical commented Oct 30, 2019

Copy link
Copy Markdown
Member Author

I submitted benchmarks to bpo

brandtbucher added the type-feature A feature request or enhancement label Oct 30, 2019
mdickinson approved these changes Oct 30, 2019
View reviewed changes

mdickinson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

serhiy-storchaka reviewed Oct 30, 2019
View reviewed changes
Comment thread Objects/floatobject.c Outdated

serhiy-storchaka commented Oct 30, 2019

Copy link
Copy Markdown
Member

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

mdickinson commented Oct 31, 2019

Copy link
Copy Markdown
Member

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 vstinner November 15, 2019 13:38

vstinner commented Dec 5, 2019

Copy link
Copy Markdown
Member

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.

vstinner commented Dec 5, 2019

Copy link
Copy Markdown
Member

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 Dec 5, 2019
View reviewed changes

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

vstinner commented Dec 5, 2019

Copy link
Copy Markdown
Member

@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?

vstinner commented Dec 14, 2019

Copy link
Copy Markdown
Member

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

mdickinson commented Dec 15, 2019

Copy link
Copy Markdown
Member

@vstinner Merging is fine with me.

vstinner merged commit cb8b946 into python:master Dec 15, 2019
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
bpo-38629: implement __floor__ and __ceil__ for float type (pythonGH-…
5d7d318 
…16985)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

vstinner vstinner approved these changes
serhiy-storchaka Awaiting requested review from serhiy-storchaka
+1 more reviewer
mdickinson mdickinson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL