Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PERF: use non-copying path for Groupby.skew #52104
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
PERF: use non-copying path for Groupby.skew #52104
Changes from 15 commits
f909a2d
293b9c0
5823f8b
ea4d54c
f94a1b3
baafbaa
4ca8bed
1f1a268
294a500
9333c78
3069f4c
bc0b67a
04acbf5
c34cdad
7e84d04
78096ed
67d2669
9609c95
703f2aa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with the rules for casting a complex to a float value, but seems potentially dangerous to do this. Is there no explicit way to get the real part from the complex and make sure the imaginary is safely discardable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no idea, and google is no help. cc @seberg thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get it, all of this is already typed as float64, no? So this uses
pow
which should already be float64 only. Isn't the cast unnecessary anyway?Otherwise, yes
.real
should exist, or.real()
if its C++ (which I don't think it is). The castwould be safe, but a bit odd.(not 100%, but then I am not sure it shouldn't be a compile error)...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the cast we got build failures on the npdev build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I had checked this line, but the problem is only in the line below :).
This must have to do with cython 3 would be my guess? It uses the Python power definition which converts to complex, but you probably want a float power anyway.
Maybe try:
<float64_t>0.5
, maybe that makes it use C power (removing the need to cast). Otherwise, maybe we can tell cython3 to use C power?EDIT: Nvm which line... it must be the Cython version only likely...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://cython.readthedocs.io/en/latest/src/userguide/source_files_and_compilation.html#compiler-directives The
cpow
directive... but dunno how easy it is to set in practice?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like
@cython.cpow
is available in cython 0.29.x, let's see if using that solves this..There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like that did it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we do this for the other functions? I like the idea; seems like it would be good to consolidate this with others in a follow up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
groupby tests are not super well-organized, would be nice to improve someday