Skip to content

Conversation

@rok
Copy link
Member

@rok rok commented Mar 29, 2021

This is to resolve ARROW-11070.

@rok
Copy link
Member Author

rok commented Mar 29, 2021

I'm not sure about two things:

  • Is Exponentiate a good name or should we go with Power? I picked Exponentiate as all the other operation names are verbs so Power feels inconsistent.
  • Should we add overflow checking logic?
  • What to do with nulls in case of power(null, 0) or power(1, null). See comment

@cyb70289
Copy link
Contributor

cyb70289 commented Mar 30, 2021

I'm not sure about two things:

* Is `Exponentiate` a good name or should we go with `Power`? I picked `Exponentiate` as all the other operation names are verbs so `Power` feels inconsistent.

I prefer power which is used widely. But I'm bad at naming, so would like to hear other comments.

* Should we add overflow checking logic?

I don't think so. Let it be infinity looks okay.
[EDIT] We do not need to check floating point overflow. But I think it's necessary for integers, like other arithmetic kernel does.

PS: Numpy checks floating point overflow, but not integer overflow. Looks strange to me.

In [2]: np.power(10, 1000)
Out[2]: 0

In [3]: np.power(10.0, 1000)
<ipython-input-20-202eedf3dc47>:1: RuntimeWarning: overflow encountered in power
  np.power(10.0, 1000)
Out[3]: inf

@github-actions
Copy link

@rok
Copy link
Member Author

rok commented Apr 1, 2021

Thanks for the feedback @cyb70289. I'll do another commit tomorrow and kindly ask for feedback after.

@rok
Copy link
Member Author

rok commented Apr 1, 2021

@cyb70289 Could you please have another look?
I've added the docs and some tests.
As for 0 raised to negative integers I'm leaning more towards the way R/C++ way as it is more general mathematically and can be de-generalized in other layers.

@cyb70289 cyb70289 changed the title ARROW-11070: [C++] Implement power / exponentiation compute kernel ARROW-11070: [C++] Implement power compute kernel Apr 4, 2021
@rok rok force-pushed the ARROW-11070 branch 2 times, most recently from 92ae75c to 29028a5 Compare April 6, 2021 22:09
@rok rok force-pushed the ARROW-11070 branch 3 times, most recently from 7f38d0e to 915904f Compare April 8, 2021 14:40
@rok
Copy link
Member Author

rok commented Apr 8, 2021

CI issues don't seem to be related.

@rok
Copy link
Member Author

rok commented Apr 8, 2021

Should we also add a benchmark? I see other kernels have them.

@cyb70289
Copy link
Contributor

cyb70289 commented Apr 9, 2021

Should we also add a benchmark? I see other kernels have them.

Yes we should. It can be in another pr.
Thanks for persistent in improving this pr. Will review soon.

@rok
Copy link
Member Author

rok commented Apr 9, 2021

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 persistent in improving this pr. Will review soon.

Thanks for the feedback! :)

Comment on lines 72 to 81
Copy link
Member Author

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?

Copy link
Contributor

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

@nealrichardson
Copy link
Member

@cyb70289 is this good to merge now? (The RTools35 CI failure has been fixed on master)

@cyb70289 cyb70289 changed the title ARROW-11070: [C++] Implement power compute kernel ARROW-11070: [C++][Compute] Implement power compute kernel Apr 13, 2021
@cyb70289 cyb70289 changed the title ARROW-11070: [C++][Compute] Implement power compute kernel ARROW-11070: [C++][Compute] Implement power kernel Apr 13, 2021
@cyb70289
Copy link
Contributor

Will merge when CI is green.

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a 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.

@cyb70289
Copy link
Contributor

cyb70289 commented Apr 13, 2021

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:

  • Base and exponent must be the same type, this is not reasonable
  • Null handling is delayed to the end, and power is always calculated even for nulls (with whatever base/exponent happen to be in the slots). This is a big waste of time for expensive power calculations.

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.

@nealrichardson
Copy link
Member

CI failure is unrelated, will merge this. @rok please link on here the followup JIRA when you make it.

@cyb70289
Copy link
Contributor

Created jira task to track improving power kernel: https://issues.apache.org/jira/browse/ARROW-12413

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants