Skip to content

/cpython

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-37798: Add C fastpath for statistics.NormalDist.inv_cdf() #15266

Open
wants to merge 1 commit into
base: master
from
+264 −73

Conversation

@corona10
Copy link
Contributor

commented Aug 14, 2019 •
edited

https://bugs.python.org/issue37798

@corona10 corona10 requested a review from python/windows-team as a code owner Aug 14, 2019

@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Aug 14, 2019

@bedevere-bot bedevere-bot added the awaiting review label Aug 14, 2019

@corona10 corona10 force-pushed the corona10:bpo-37798 branch 3 times, most recently from 8f96826 to fa18af8 Aug 14, 2019

@tirkarthi

tirkarthi reviewed Aug 14, 2019
View changes

Lib/statistics.py Outdated Show resolved Hide resolved

@corona10 corona10 changed the title bpo-37998: Add C fastpath for statistics.NormalDist.inv_cdf() bpo-37798: Add C fastpath for statistics.NormalDist.inv_cdf() Aug 14, 2019

@corona10 corona10 force-pushed the corona10:bpo-37798 branch 4 times, most recently from 5a77376 to ba81a90 Aug 14, 2019

@rhettinger

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Aug 15, 2019

The usual way we do this is to define the function in pure python. Then, somewhere below that definition, try to import the C accelerator function in its place. For examples, see line 458 in Lib/collections/__init__.py for _count_elements() or line 579 in Lib/heapq.py

@rhettinger rhettinger self-assigned this Aug 15, 2019

@corona10 corona10 force-pushed the corona10:bpo-37798 branch from ba81a90 to 9663297 Aug 15, 2019

@corona10

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Aug 15, 2019

@rhettinger

Thanks a lot.
I 've updated the PR as you suggested.
I both test when using pure python version and C version.
And it looks same on both versions.
Please take a look.
@rhettinger

rhettinger reviewed Aug 15, 2019
View changes

Lib/statistics.py Outdated Show resolved Hide resolved

@corona10 corona10 force-pushed the corona10:bpo-37798 branch from 9663297 to ccec88f Aug 15, 2019

@rhettinger

rhettinger requested changes Aug 15, 2019
View changes

Modules/_statisticsmodule.c Outdated Show resolved Hide resolved

@bedevere-bot bedevere-bot removed the awaiting review label Aug 15, 2019

@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

commented Aug 15, 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 bedevere-bot added the awaiting changes label Aug 15, 2019

@corona10 corona10 force-pushed the corona10:bpo-37798 branch from ccec88f to ba6bc56 Aug 15, 2019

@corona10

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Aug 15, 2019

I have made the requested changes; please review again

@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Aug 15, 2019

@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

commented Aug 15, 2019

Thanks for making the requested changes!

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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Aug 21, 2019

@rhettinger Can you please take a look?
@rhettinger

rhettinger requested changes Aug 22, 2019
View changes

Lib/statistics.py Outdated Show resolved Hide resolved
Modules/_statisticsmodule.c Outdated Show resolved Hide resolved
Modules/_statisticsmodule.c Outdated Show resolved Hide resolved
Modules/_statisticsmodule.c Show resolved Hide resolved

@bedevere-bot bedevere-bot removed the awaiting change review label Aug 22, 2019

@bedevere-bot bedevere-bot added the awaiting changes label Aug 22, 2019

@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

commented Aug 22, 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.
@rhettinger

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Aug 22, 2019

Thanks for doing this. I think we're getting close :-)
@corona10

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Aug 22, 2019

Thanks for the review, I will take look at!

@corona10 corona10 force-pushed the corona10:bpo-37798 branch from ba6bc56 to a921c83 Aug 23, 2019

@corona10 corona10 force-pushed the corona10:bpo-37798 branch from a921c83 to c004470 Aug 23, 2019

@corona10

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Aug 23, 2019

@rhettinger I've updated the PR!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tirkarthi tirkarthi

@rhettinger rhettinger

@python/windows-team windows-team

Assignees

@rhettinger rhettinger

Labels
CLA signed awaiting changes
Projects
None yet
Milestone
No milestone
5 participants
@corona10 @rhettinger @bedevere-bot @tirkarthi @the-knights-who-say-ni
You can’t perform that action at this time.