GH-50197: [C++][Python] Add "hypot" compute kernel#50198
Conversation
|
|
1978af2 to
9568a77
Compare
|
i rebased on main @pitrou could you check this out |
|
This looks good. Consider adding arrow/docs/source/python/api/compute.rst Lines 161 to 181 in 12b3eda |
9568a77 to
7e74a4c
Compare
added hypot to the python compute docs and rebased on main. Thanks for the review! |
|
@shrivasshankar I don't think the failures are related. Could you try rebasing? |
7e74a4c to
bd4b80c
Compare
I rebased and it has to rerun |
Add a binary floating-point compute function "hypot" that computes the hypotenuse sqrt(x^2 + y^2) without undue overflow or underflow at intermediate stages, mirroring numpy.hypot and std::hypot. The kernel reuses the existing MakeArithmeticFunctionFloatingPoint factory (as atan2 does), registering float32 and float64 kernels and promoting integer/decimal inputs to float64 via DispatchBest. Also adds the Hypot() convenience function and declaration, a FunctionDoc, C++ tests (including an overflow-safety test that exercises inputs whose squares overflow float32), a compute.rst entry, and pyarrow expression coverage.
bd4b80c to
6168525
Compare
rok
left a comment
There was a problem hiding this comment.
I think this is good to go if CI passes.
Co-authored-by: Rok Mihevc <rok@mihevc.org>
sounds good applying ur suggestion reset the CI again |
|
@rok all pass besides 1 which seems to be a runner crash not a test failure |
|
@pitrou I think this is ready to go, if you can please take a final look, I'd like to merge for this release. |
|
@shrivasshankar would you consider adding two tests? |
rok
left a comment
There was a problem hiding this comment.
Would like to add the two test cases before merging.
Co-authored-by: Rok Mihevc <rok@mihevc.org>
Co-authored-by: Rok Mihevc <rok@mihevc.org>
|
I added the 2 tests @rok |
|
Thanks for working on this @shrivasshankar ! |
Rationale for this change
Arrow has
atan2andlogbbut nohypot. Computing a Euclidean norm currently needssqrt(add(multiply(x, x), multiply(y, y))), which can overflow at themultiplystep even when the result is representable.What changes are included in this PR?
Add a
hypotkernel backed bystd::hypot(float32/float64, with integer/decimal inputs promoted to float64), theHypot()C++ function, aFunctionDocandcompute.rstentry, and a Python expression test.pc.hypotis auto-exposed from the registry.Are these changes tested?
Yes, C++ tests for the basic cases, null/NaN/Inf handling, and an overflow case confirming it doesn't fall back to a naive
sqrt(x*x + y*y).Are there any user-facing changes?
Yes, a new
hypotfunction in C++ and Python. Additive only.