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

Open
wants to merge 1 commit into
base: master
from

Conversation

@corona10
Copy link
Contributor

commented Aug 14, 2019

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

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

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.

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.

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.

Lib/statistics.py Outdated Show resolved Hide resolved

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

Modules/_statisticsmodule.c Outdated Show resolved Hide resolved
@bedevere-bot

This comment has been minimized.

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.

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

@corona10

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

I have made the requested changes; please review again

@bedevere-bot

This comment has been minimized.

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.

Copy link
Contributor Author

commented Aug 21, 2019

@rhettinger Can you please take a look?

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

This comment has been minimized.

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.

Copy link
Contributor

commented Aug 22, 2019

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

@corona10

This comment has been minimized.

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.

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
Projects
None yet
5 participants
You can’t perform that action at this time.