Skip to content

PERF: Using Numpy C-API when calling np.arange #32804

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

Merged
merged 1 commit into from
Mar 18, 2020

Conversation

ShaharNaveh
Copy link
Member

@ShaharNaveh ShaharNaveh commented Mar 18, 2020

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

Somewhat of a follow up to #32681.


I could not benchmark this change as this function is cdef and not def or cpdef (I would also love if someone could give me a tip on how to benchmark cdef functions from an Ipython shell for example).

What I did what I ran cython -a pandas/_libs/internals.pyx and took a screenshot of the before and after.


Master:

orig_arange


PR:

New_arange


Also, is there a reason not to replace every call of np.arange with cnp.PyArray_Arange? (cython files only)

@simonjayhawkins simonjayhawkins added the Performance Memory or execution speed performance label Mar 18, 2020
@jbrockmendel
Copy link
Member

I would also love if someone could give me a tip on how to benchmark cdef functions from an Ipython shell for example

i usually write a def wrapper in the same file to call the cdef function in a loop. You could also find the "closest" non-cdef function that calls the affected function and profile that

@jbrockmendel
Copy link
Member

Also, is there a reason not to replace every call of np.arange with cnp.PyArray_Arange? (cython files only)

if its in a place that gets called a zillion times (especially in a loop), then go for it.

For others is probably isn't worth the cognitive overhead for the next reader to come along.

@ShaharNaveh
Copy link
Member Author

Also, is there a reason not to replace every call of np.arange with cnp.PyArray_Arange? (cython files only)

if its in a place that gets called a zillion times (especially in a loop), then go for it.

Okay, make sense.

For others is probably isn't worth the cognitive overhead for the next reader to come along.

If a comment is included (like in this PR), does it solves the cognitive overhead?

and are the there any other objections?

@jbrockmendel
Copy link
Member

If a comment is included (like in this PR), does it solves the cognitive overhead?

I guess, but then it feels fishy to be adding identical comments in many places (then again, I do this too). Your call.

Separate topic: in core.groupby.generic L389, L1350, L1702 there are calls to ._convert that I think are superfluous. Getting a second pair of eyes to try to reason about the code and prove it would be great if you're up for it.

@jbrockmendel jbrockmendel merged commit 095998c into pandas-dev:master Mar 18, 2020
@jbrockmendel
Copy link
Member

thanks @MomIsBestFriend

@ShaharNaveh ShaharNaveh deleted the PERF-arange-diss branch March 18, 2020 20:42
@jorisvandenbossche
Copy link
Member

I guess, but then it feels fishy to be adding identical comments in many places (then again, I do this too). Your call.

BTW, I don't think we should merge such PRs in the future without a performance check showing an considerable improvement.
(as I agree it gives cognitive overhead, so we should only do it when worth it; what is worth it is of course subjective, but at least we should time it)

@simonjayhawkins simonjayhawkins added this to the 1.1 milestone Mar 18, 2020
SeeminSyed pushed a commit to CSCD01-team01/pandas that referenced this pull request Mar 22, 2020
jbrockmendel pushed a commit to jbrockmendel/pandas that referenced this pull request Mar 23, 2020
jbrockmendel pushed a commit to jbrockmendel/pandas that referenced this pull request Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Performance Memory or execution speed performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants