-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-11070: [C++][Compute] Implement power kernel #9841
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
Conversation
|
I'm not sure about two things:
|
I prefer
I don't think so. Let it be PS: Numpy checks floating point overflow, but not integer overflow. Looks strange to me. |
|
Thanks for the feedback @cyb70289. I'll do another commit tomorrow and kindly ask for feedback after. |
|
@cyb70289 Could you please have another look? |
92ae75c to
29028a5
Compare
7f38d0e to
915904f
Compare
|
CI issues don't seem to be related. |
|
Should we also add a benchmark? I see other kernels have them. |
Yes we should. It can be in another pr. |
I've added the benchmark. I hope I've not missed something there.
Thanks for the feedback! :) |
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 had to reduce the range to avoid overflows. Would it make more sense to introduce a separate generator for power benchmark only?
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 tweaked some code and removed the benchmark.
For now, I think there are still many things needs to improve. So we don't need to add benchmark in a haste.
I downloaded and attached your benchmark commit, in case it's lost.
0001-Adding-benchmark-for-Power-and-PowerChecked.patch.gz
|
@cyb70289 is this good to merge now? (The RTools35 CI failure has been fixed on master) |
|
Will merge when CI is green. |
jorisvandenbossche
left a comment
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 added a few comments on the docstring of the function. But feel free to merge when green, and this can also be done in a follow-up.
|
Record things to improve, will create Jira to track. Power kernel is modified from add/sub/mul/div kernels. But power is much more complicated than basic arithmetic operations. Some limitations in current implementation:
For checked power, integer overflow is checked for every multiplication. As the integer upper bound is already know, it can be calculate directly if the power will overflow. It will remove overflow checks in each iteration, but will introduce one expensive logarithm operation. Maybe deserve a try. |
|
CI failure is unrelated, will merge this. @rok please link on here the followup JIRA when you make it. |
|
Created jira task to track improving power kernel: https://issues.apache.org/jira/browse/ARROW-12413 |
This is to resolve ARROW-11070.