-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
GH-44952: [C++][Python] Add Hyperbolic Trig functions #44630
Conversation
|
Hi @amoeba just wanted to check in if you had a chance to review this commit. The test failure is very strange (on an encryption test?!), but I can look into it! |
This test is known to be flaky (#43057), the failure is unrelated to the changes in this PR. |
Hi @khwilson, thanks for the ping. I'll take a look in the next couple of days. |
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.
Thanks for contributing this. I left some notes inline.
Beyond that, can you please update the Python API docs to list these new functions?
(See dfb0928#diff-ce5b94577014735990903d3d03bd4ea4b8c8e6d32f5227592e60b7dd6a912d59R342 for an example.)
@felipecrv do you have any time to give this a second review? |
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.
This is very well done. I found a simple range check error and suggested documentation improvements.
|
Thanks for the reviews. I can:
Remaining question for reviewers. In C++, Which behaviour should this library (which spans both C++ and Python) utilize? |
Answered in the respective threads. Summary: -Inf/Inf on the unchecked version, range error on the checked version. |
Makes sense. Will implement. |
OK, I believe all the comments were addressed. Now to wait for tests and likely rebase :-) |
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 can merge soon if no one has more changes to request.
fcc3ec2
to
2acbf5a
Compare
Also rebased onto main as a bunch of tests seem to be failing for reasons I'm guessing are due to being slightly out of date :-) |
Huh, looks like azurite is currently 404-ing, which is why some of the tests are failing. Seems to be due to a scheduled maintenance window as react is also 404-ing. Will rerun tests later. |
I'm keeping an eye on them and will re-run the unrelated failures as needed. |
@khwilson: This looks like a real failure: https://github.com/apache/arrow/actions/runs/12224231650/job/34096726088?pr=44630#step:6:3100. I think that can be fixed by adding a |
Added. Ran the main tests locally, but didn't run the doc tests it seems. |
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.
LGTM and CI is green. I'll let @felipecrv merge.
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 104b040. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Rationale for this change
Hyperbolic trigonometric functions are a common transformation for dealing with skewed data. And they are built into the core C++ libraries so require minimal change.
What changes are included in this PR?
Adding
a?(sin|cos|tan)h
to the base C++ library and substrait and tests for these functions in pyarrow.Are these changes tested?
Yes, in the same style as the trigonometric functions.
Are there any user-facing changes?
Yes. Additional compute functions are added.