You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "khwilson (via GitHub)" <gi...@apache.org> on 2024/11/03 20:35:42 UTC
[PR] GH-44376: [C++][Python] Add Hyperbolic Trig functions [arrow]
khwilson opened a new pull request, #44630:
URL: https://github.com/apache/arrow/pull/44630
### 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "khwilson (via GitHub)" <gi...@apache.org>.
khwilson commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1871711724
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -273,6 +328,29 @@ struct Atan {
}
};
+struct Atanh {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
Review Comment:
I actually went back and forth on this. In *python* they raise a domain error. In the C++ stdlib, they return ± infinity and raise an error. (which I realize I didn't clear....). See [here](https://en.cppreference.com/w/cpp/numeric/math/atanh).
Which library this PR should be consistent with?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1871954422
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -273,6 +328,29 @@ struct Atan {
}
};
+struct Atanh {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
Review Comment:
FWIW, a common approach when implementing Acero functions in cases like this has been to add options to allow the user to pick what behavior they want. It might be worth doing that here if it doesn't add excessive complexity or inefficiency.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "khwilson (via GitHub)" <gi...@apache.org>.
khwilson commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1871711724
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -273,6 +328,29 @@ struct Atan {
}
};
+struct Atanh {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
Review Comment:
I actually went back and forth on this. In *python* they raise a domain error. In the C++ stdlib, they return ± infinity and raise a divide by zero error. (which I realize I didn't clear....). See [here](https://en.cppreference.com/w/cpp/numeric/math/atanh).
Which library this PR should be consistent with?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1871938573
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -273,6 +328,29 @@ struct Atan {
}
};
+struct Atanh {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::atanh(val);
+ }
+};
+
+struct AtanhChecked {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status* st) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
Review Comment:
The checked version should reject -1.0 and 1.0. Justification in the previous thread.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1871954422
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -273,6 +328,29 @@ struct Atan {
}
};
+struct Atanh {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
Review Comment:
FWIW, a common approach when implementing Acero functions in cases like this has been to add options to allow the user can pick what behavior they want. It might be worth doing that here if it doesn't add excessive complexity or inefficiency.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "khwilson (via GitHub)" <gi...@apache.org>.
khwilson commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1872116052
##########
cpp/src/arrow/compute/api_scalar.cc:
##########
@@ -742,6 +744,10 @@ SCALAR_ARITHMETIC_UNARY(Sqrt, "sqrt", "sqrt_checked")
SCALAR_ARITHMETIC_UNARY(Negate, "negate", "negate_checked")
SCALAR_ARITHMETIC_UNARY(Sin, "sin", "sin_checked")
SCALAR_ARITHMETIC_UNARY(Tan, "tan", "tan_checked")
+SCALAR_EAGER_UNARY(Asinh, "asinh")
+SCALAR_EAGER_UNARY(Cosh, "cosh")
+SCALAR_EAGER_UNARY(Sinh, "sinh")
+SCALAR_EAGER_UNARY(Tanh, "tanh")
SCALAR_EAGER_UNARY(Atan, "atan")
SCALAR_EAGER_UNARY(Exp, "exp")
SCALAR_EAGER_UNARY(Sign, "sign")
Review Comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1872311375
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -273,6 +328,29 @@ struct Atan {
}
};
+struct Atanh {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
Review Comment:
Thanks for digging in all. I think the direction this conversion went makes sense (unchecked version returns -Inf/Inf, checked version raises). I was checking these in R and SQLite and it looks like Python is sort of the odd man out here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "khwilson (via GitHub)" <gi...@apache.org>.
khwilson commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1871712422
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -273,6 +328,29 @@ struct Atan {
}
};
+struct Atanh {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::atanh(val);
+ }
+};
+
+struct AtanhChecked {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status* st) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
Review Comment:
Same comment as the previous one
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1871937532
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -273,6 +328,29 @@ struct Atan {
}
};
+struct Atanh {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
Review Comment:
C++ docs also say `If the argument is ±1, a pole error occurs.` and `if the argument is ±1, ±∞ is returned and [FE_DIVBYZERO] is raised.
They have different names for the errors (range/pole/FE_DIVBYZERO) but that doesn't change the fact that Mathematically, `atanh` isn't defined at the -1.0 and 1.0 points.
The function **converges to** $\pm\infty$ as $x$ approaches the extremes of the range which makes returning `-Inf/+Inf` more reasonable than returning `NaN` at these extremes, but it's not defined at these points per se.
$$\lim_{x \to 1.0}{tanh^{-1}(x)} = \infty$$
$$\lim_{x \to -1.0}{tanh^{-1}(x)} = -\infty$$
So I think you can drop my suggestion on the unchecked version (this one) and let the implementation return `-Inf/+Inf` instead of `NaN` at these extremes, but the `atanh_checked` variation should fail at `-1.0/+1.0` instead of returning the infinite values.
~Docs should be updated accordingly to describe this behavior.~
I updated my suggestions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "khwilson (via GitHub)" <gi...@apache.org>.
khwilson commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1871711724
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -273,6 +328,29 @@ struct Atan {
}
};
+struct Atanh {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
Review Comment:
I actually went back and forth on this. In *python* they raise a domain error. In the C++ stdlib, they return ± infinity. So the code as is returns ± infinity and an error is raised (which I realize I didn't clear....). See [here](https://en.cppreference.com/w/cpp/numeric/math/atanh).
Which library this PR should be consistent with
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1871939981
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1212,6 +1298,16 @@ const FunctionDoc acos_checked_doc{"Compute the inverse cosine",
"to return NaN instead, see \"acos\"."),
{"x"}};
+const FunctionDoc acosh_doc{"Compute the inverse hyperbolic cosine",
+ ("NaN is returned for invalid input values;\n"
Review Comment:
No need. Just make the ones you are adding more informative.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1871505691
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc:
##########
@@ -2476,6 +2541,19 @@ TYPED_TEST(TestBinaryArithmeticFloating, TrigAtan2) {
-M_PI_2, 0, M_PI));
}
+TYPED_TEST(TestUnaryArithmeticFloating, TrigAtanh) {
+ this->SetNansEqual(true);
+ this->AssertUnaryOp(Atanh, "[-Inf, Inf, -2, 2]", "[NaN, NaN, NaN, NaN]");
Review Comment:
~add -1 and 1 as expecting NaN as well.~
add -1 and 1 as expecting -Inf and Inf
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1871937532
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -273,6 +328,29 @@ struct Atan {
}
};
+struct Atanh {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
Review Comment:
C++ docs also say `If the argument is ±1, a pole error occurs.` and `if the argument is ±1, ±∞ is returned and [FE_DIVBYZERO] is raised.`
They have different names for the errors (range/pole/FE_DIVBYZERO) but that doesn't change the fact that Mathematically, `atanh` isn't defined at the -1.0 and 1.0 points.
The function **converges to** $\pm\infty$ as $x$ approaches the extremes of the range which makes returning `-Inf/+Inf` more reasonable than returning `NaN` at these extremes, but it's not defined at these points per se.
$$\lim_{x \to 1.0}{tanh^{-1}(x)} = \infty$$
$$\lim_{x \to -1.0}{tanh^{-1}(x)} = -\infty$$
So I think you can drop my suggestion on the unchecked version (this one) and let the implementation return `-Inf/+Inf` instead of `NaN` at these extremes, but the `atanh_checked` variation should fail at `-1.0/+1.0` instead of returning the infinity values.
~Docs should be updated accordingly to describe this behavior.~
I updated my suggestions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1871954962
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -273,6 +328,29 @@ struct Atan {
}
};
+struct Atanh {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
Review Comment:
The logarithms definition of `atanh` makes the issue more apparent:
$$tanh^{-1}(x) = \frac{1}{2}\ln\frac{1 + x}{1 - x}$$
If x is 1, you get a division by 0. If x is -1 you get $\ln(0)$ which is undefined (converges to `-Inf`, but not defined).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "khwilson (via GitHub)" <gi...@apache.org>.
khwilson commented on PR #44630:
URL: https://github.com/apache/arrow/pull/44630#issuecomment-2521206762
Makes sense. Will implement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44376: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #44630:
URL: https://github.com/apache/arrow/pull/44630#issuecomment-2453569053
:warning: GitHub issue #44376 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1871492470
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1221,6 +1317,16 @@ const FunctionDoc atan2_doc{"Compute the inverse tangent of y/x",
("The return value is in the range [-pi, pi]."),
{"y", "x"}};
+const FunctionDoc atanh_doc{"Compute the inverse hyperbolic tangent",
+ ("NaN is returned for invalid input values;\n"
+ "to raise an error instead, see \"atanh_checked\"."),
+ {"x"}};
+
+const FunctionDoc atanh_checked_doc{"Compute the inverse hyperbolic tangent",
+ ("Invalid input values raise an error;\n"
+ "to return NaN instead, see \"atanh\"."),
+ {"x"}};
Review Comment:
```suggestion
const FunctionDoc atanh_checked_doc{"Compute the inverse hyperbolic tangent",
("Input values outside the (-1.0, 1.0) range raise an error;\n"
"to return NaN, -Inf, and Inf instead, see \"atanh\"."),
{"x"}};
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "khwilson (via GitHub)" <gi...@apache.org>.
khwilson commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1872081298
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1172,6 +1250,8 @@ const FunctionDoc sin_checked_doc{"Compute the sine",
"to return NaN instead, see \"sin\"."),
{"x"}};
+const FunctionDoc sinh_doc{"Compute the hyperblic sine", (""), {"x"}};
Review Comment:
Whoops.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "khwilson (via GitHub)" <gi...@apache.org>.
khwilson commented on PR #44630:
URL: https://github.com/apache/arrow/pull/44630#issuecomment-2526309627
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 :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "khwilson (via GitHub)" <gi...@apache.org>.
khwilson commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1872144021
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -273,6 +328,29 @@ struct Atan {
}
};
+struct Atanh {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
Review Comment:
Notably, C++ defines [tanh(oo) = 1](https://en.cppreference.com/w/cpp/numeric/math/tanh)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "khwilson (via GitHub)" <gi...@apache.org>.
khwilson commented on PR #44630:
URL: https://github.com/apache/arrow/pull/44630#issuecomment-2526338632
Added. Ran the main tests locally, but didn't run the doc tests it seems.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44376: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on PR #44630:
URL: https://github.com/apache/arrow/pull/44630#issuecomment-2496259049
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on PR #44630:
URL: https://github.com/apache/arrow/pull/44630#issuecomment-2526316371
I'm keeping an eye on them and will re-run the unrelated failures as needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "khwilson (via GitHub)" <gi...@apache.org>.
khwilson commented on PR #44630:
URL: https://github.com/apache/arrow/pull/44630#issuecomment-2526300999
Well, that's embarrassing. Pushed the fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "khwilson (via GitHub)" <gi...@apache.org>.
khwilson commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1871714072
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1212,6 +1298,16 @@ const FunctionDoc acos_checked_doc{"Compute the inverse cosine",
"to return NaN instead, see \"acos\"."),
{"x"}};
+const FunctionDoc acosh_doc{"Compute the inverse hyperbolic cosine",
+ ("NaN is returned for invalid input values;\n"
Review Comment:
I was following the convention of the other inverse trig functions. Should we switch their docs at the same time to specify their ranges?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1871491347
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1221,6 +1317,16 @@ const FunctionDoc atan2_doc{"Compute the inverse tangent of y/x",
("The return value is in the range [-pi, pi]."),
{"y", "x"}};
+const FunctionDoc atanh_doc{"Compute the inverse hyperbolic tangent",
+ ("NaN is returned for invalid input values;\n"
+ "to raise an error instead, see \"atanh_checked\"."),
+ {"x"}};
Review Comment:
```suggestion
const FunctionDoc atanh_doc{"Compute the inverse hyperbolic tangent",
("NaN is returned for input values outside of the [-1.0, 1.0] range;-Inf and Inf are returned for -1.0 and 1.0 respectively;\n"
"to raise an error instead, see \"atanh_checked\"."),
{"x"}};
```
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1221,6 +1317,16 @@ const FunctionDoc atan2_doc{"Compute the inverse tangent of y/x",
("The return value is in the range [-pi, pi]."),
{"y", "x"}};
+const FunctionDoc atanh_doc{"Compute the inverse hyperbolic tangent",
+ ("NaN is returned for invalid input values;\n"
+ "to raise an error instead, see \"atanh_checked\"."),
+ {"x"}};
Review Comment:
Updated the suggestion to mention -Inf/Inf
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #44630:
URL: https://github.com/apache/arrow/pull/44630#issuecomment-2520531896
:warning: GitHub issue #44952 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44376: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1871487027
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1212,6 +1298,16 @@ const FunctionDoc acos_checked_doc{"Compute the inverse cosine",
"to return NaN instead, see \"acos\"."),
{"x"}};
+const FunctionDoc acosh_doc{"Compute the inverse hyperbolic cosine",
+ ("NaN is returned for invalid input values;\n"
+ "to raise an error instead, see \"acosh_checked\"."),
+ {"x"}};
+
+const FunctionDoc acosh_checked_doc{"Compute the inverse hyperbolic cosine",
+ ("Invalid input values raise an error;\n"
+ "to return NaN instead, see \"acosh\"."),
+ {"x"}};
Review Comment:
```suggestion
const FunctionDoc acosh_checked_doc{"Compute the inverse hyperbolic cosine",
("Input values < 1.0 raise an error;\n"
"to return NaN instead, see \"acosh\"."),
{"x"}};
```
##########
cpp/src/arrow/compute/api_scalar.cc:
##########
@@ -742,6 +744,10 @@ SCALAR_ARITHMETIC_UNARY(Sqrt, "sqrt", "sqrt_checked")
SCALAR_ARITHMETIC_UNARY(Negate, "negate", "negate_checked")
SCALAR_ARITHMETIC_UNARY(Sin, "sin", "sin_checked")
SCALAR_ARITHMETIC_UNARY(Tan, "tan", "tan_checked")
+SCALAR_EAGER_UNARY(Asinh, "asinh")
+SCALAR_EAGER_UNARY(Cosh, "cosh")
+SCALAR_EAGER_UNARY(Sinh, "sinh")
+SCALAR_EAGER_UNARY(Tanh, "tanh")
SCALAR_EAGER_UNARY(Atan, "atan")
SCALAR_EAGER_UNARY(Exp, "exp")
SCALAR_EAGER_UNARY(Sign, "sign")
Review Comment:
Can you keep all of these sorted alphabetically? There is currently only one violation of that in the existing code (`Sqrt`):
```diff
SCALAR_ARITHMETIC_UNARY(AbsoluteValue, "abs", "abs_checked")
SCALAR_ARITHMETIC_UNARY(Acos, "acos", "acos_checked")
+SCALAR_ARITHMETIC_UNARY(Acosh, "acosh", "acosh_checked")
SCALAR_ARITHMETIC_UNARY(Asin, "asin", "asin_checked")
+SCALAR_ARITHMETIC_UNARY(Atanh, "atanh", "atanh_checked")
SCALAR_ARITHMETIC_UNARY(Cos, "cos", "cos_checked")
SCALAR_ARITHMETIC_UNARY(Ln, "ln", "ln_checked")
SCALAR_ARITHMETIC_UNARY(Log10, "log10", "log10_checked")
SCALAR_ARITHMETIC_UNARY(Log1p, "log1p", "log1p_checked")
SCALAR_ARITHMETIC_UNARY(Log2, "log2", "log2_checked")
-SCALAR_ARITHMETIC_UNARY(Sqrt, "sqrt", "sqrt_checked")
SCALAR_ARITHMETIC_UNARY(Negate, "negate", "negate_checked")
SCALAR_ARITHMETIC_UNARY(Sin, "sin", "sin_checked")
+SCALAR_ARITHMETIC_UNARY(Sqrt, "sqrt", "sqrt_checked")
SCALAR_ARITHMETIC_UNARY(Tan, "tan", "tan_checked")
+SCALAR_EAGER_UNARY(Asinh, "asinh")
SCALAR_EAGER_UNARY(Atan, "atan")
+SCALAR_EAGER_UNARY(Cosh, "cosh")
SCALAR_EAGER_UNARY(Exp, "exp")
SCALAR_EAGER_UNARY(Expm1, "expm1")
SCALAR_EAGER_UNARY(Sign, "sign")
+SCALAR_EAGER_UNARY(Sinh, "sinh")
+SCALAR_EAGER_UNARY(Tanh, "tanh")
```
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1172,6 +1250,8 @@ const FunctionDoc sin_checked_doc{"Compute the sine",
"to return NaN instead, see \"sin\"."),
{"x"}};
+const FunctionDoc sinh_doc{"Compute the hyperblic sine", (""), {"x"}};
Review Comment:
```suggestion
const FunctionDoc sinh_doc{"Compute the hyperbolic sine", (""), {"x"}};
```
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -273,6 +328,29 @@ struct Atan {
}
};
+struct Atanh {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
Review Comment:
```suggestion
if (ARROW_PREDICT_FALSE((val <= -1.0 || val => 1.0))) {
```
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc:
##########
@@ -2476,6 +2541,19 @@ TYPED_TEST(TestBinaryArithmeticFloating, TrigAtan2) {
-M_PI_2, 0, M_PI));
}
+TYPED_TEST(TestUnaryArithmeticFloating, TrigAtanh) {
+ this->SetNansEqual(true);
+ this->AssertUnaryOp(Atanh, "[-Inf, Inf, -2, 2]", "[NaN, NaN, NaN, NaN]");
Review Comment:
add -1 and 1 as expecting NaN as well.
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1221,6 +1317,16 @@ const FunctionDoc atan2_doc{"Compute the inverse tangent of y/x",
("The return value is in the range [-pi, pi]."),
{"y", "x"}};
+const FunctionDoc atanh_doc{"Compute the inverse hyperbolic tangent",
+ ("NaN is returned for invalid input values;\n"
+ "to raise an error instead, see \"atanh_checked\"."),
+ {"x"}};
Review Comment:
```suggestion
const FunctionDoc atanh_doc{"Compute the inverse hyperbolic tangent",
("NaN is returned for input values outside of the (-1.0, 1.0) range;\n"
"to raise an error instead, see \"atanh_checked\"."),
{"x"}};
```
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc:
##########
@@ -2401,6 +2405,19 @@ TYPED_TEST(TestUnaryArithmeticFloating, TrigSin) {
this->AssertUnaryOpRaises(Sin, "[Inf, -Inf]", "domain error");
}
+TYPED_TEST(TestUnaryArithmeticFloating, TrigSinh) {
+ this->SetNansEqual(true);
+ auto sinh = [](const Datum& arg, ArithmeticOptions, ExecContext* ctx) {
+ return Sinh(arg, ctx);
+ };
+
+ this->SetNansEqual(true);
Review Comment:
```suggestion
```
Already set on the first line.
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1221,6 +1317,16 @@ const FunctionDoc atan2_doc{"Compute the inverse tangent of y/x",
("The return value is in the range [-pi, pi]."),
{"y", "x"}};
+const FunctionDoc atanh_doc{"Compute the inverse hyperbolic tangent",
+ ("NaN is returned for invalid input values;\n"
+ "to raise an error instead, see \"atanh_checked\"."),
+ {"x"}};
+
+const FunctionDoc atanh_checked_doc{"Compute the inverse hyperbolic tangent",
+ ("Invalid input values raise an error;\n"
+ "to return NaN instead, see \"atanh\"."),
+ {"x"}};
Review Comment:
```suggestion
const FunctionDoc atanh_checked_doc{"Compute the inverse hyperbolic tangent",
("Input values outside the (-1.0, 1.0) range raise an error;\n"
"to return NaN instead, see \"atanh\"."),
{"x"}};
```
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -273,6 +328,29 @@ struct Atan {
}
};
+struct Atanh {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::atanh(val);
+ }
+};
+
+struct AtanhChecked {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status* st) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
Review Comment:
```suggestion
if (ARROW_PREDICT_FALSE((val <= -1.0 || val => 1.0))) {
```
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -273,6 +328,29 @@ struct Atan {
}
};
+struct Atanh {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
Review Comment:
No, it's actually a domain error:
```
Cell In[24], line 1
----> 1 math.atanh(-1.0)
ValueError: math domain error
In [25]: math.atanh(1.0)
---------------------------------------------------------------------------
ValueError Traceback (most recent call last)
Cell In[25], line 1
----> 1 math.atanh(1.0)
ValueError: math domain error
```
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1212,6 +1298,16 @@ const FunctionDoc acos_checked_doc{"Compute the inverse cosine",
"to return NaN instead, see \"acos\"."),
{"x"}};
+const FunctionDoc acosh_doc{"Compute the inverse hyperbolic cosine",
+ ("NaN is returned for invalid input values;\n"
Review Comment:
```suggestion
("NaN is returned for input values < 1.0;\n"
```
##########
cpp/src/arrow/compute/api_scalar.cc:
##########
@@ -742,6 +744,10 @@ SCALAR_ARITHMETIC_UNARY(Sqrt, "sqrt", "sqrt_checked")
SCALAR_ARITHMETIC_UNARY(Negate, "negate", "negate_checked")
SCALAR_ARITHMETIC_UNARY(Sin, "sin", "sin_checked")
SCALAR_ARITHMETIC_UNARY(Tan, "tan", "tan_checked")
+SCALAR_EAGER_UNARY(Asinh, "asinh")
+SCALAR_EAGER_UNARY(Cosh, "cosh")
+SCALAR_EAGER_UNARY(Sinh, "sinh")
+SCALAR_EAGER_UNARY(Tanh, "tanh")
SCALAR_EAGER_UNARY(Atan, "atan")
SCALAR_EAGER_UNARY(Exp, "exp")
SCALAR_EAGER_UNARY(Sign, "sign")
Review Comment:
For anyone reading in the future: I double checked the list of functions and their range restrictions (including handling of `Inf`) and the set of `_checked` functions added here is correct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1871937532
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -273,6 +328,29 @@ struct Atan {
}
};
+struct Atanh {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
Review Comment:
C++ docs also say `If the argument is ±1, a pole error occurs.` and `if the argument is ±1, ±∞ is returned and [FE_DIVBYZERO] is raised.
They have different names for the errors (range/pole/FE_DIVBYZERO) but that doesn't change the fact that Mathematically, `atanh` isn't defined at the -1.0 and 1.0 points.
The function **converges to** $\pm\infty$ as $x$ approaches the extremes of the range which makes returning `-Inf/+Inf` more reasonable than returning `NaN` at these extremes, but it's not defined at these points per se.
$$\lim_{x \to 1.0}{tanh^{-1}(x)} = \infty$$
$$\lim_{x \to -1.0}{tanh^{-1}(x)} = -\infty$$
So I think you can drop my suggestion on the unchecked version (this one) and let the implementation return `-Inf/+Inf` instead of `NaN` at these extremes, but the `atanh_checked` variation should fail at `-1.0/+1.0` instead of returning the infinite values.
Docs should be updated accordingly to describe this behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #44630:
URL: https://github.com/apache/arrow/pull/44630#issuecomment-2521198668
> Remaining question for reviewers. In C++, `atanh(1) = + infinity` (and sets a divide by zero error). In Python `atanh(1)` is a domain error.
>
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv merged PR #44630:
URL: https://github.com/apache/arrow/pull/44630
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44376: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on PR #44630:
URL: https://github.com/apache/arrow/pull/44630#issuecomment-2508717530
Hi @khwilson, thanks for the ping. I'll take a look in the next couple of days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "khwilson (via GitHub)" <gi...@apache.org>.
khwilson commented on PR #44630:
URL: https://github.com/apache/arrow/pull/44630#issuecomment-2526314232
Huh, looks like azurite is [currently 404-ing](https://www.npmjs.com/package/azurite), which is why some of the tests are failing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "khwilson (via GitHub)" <gi...@apache.org>.
khwilson commented on PR #44630:
URL: https://github.com/apache/arrow/pull/44630#issuecomment-2521812387
OK, I believe all the comments were addressed. Now to wait for tests and likely rebase :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "khwilson (via GitHub)" <gi...@apache.org>.
khwilson commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1872375260
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -273,6 +328,29 @@ struct Atan {
}
};
+struct Atanh {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
Review Comment:
Sounds good. Change made.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "khwilson (via GitHub)" <gi...@apache.org>.
khwilson commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1872380050
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1212,6 +1298,16 @@ const FunctionDoc acos_checked_doc{"Compute the inverse cosine",
"to return NaN instead, see \"acos\"."),
{"x"}};
+const FunctionDoc acosh_doc{"Compute the inverse hyperbolic cosine",
+ ("NaN is returned for invalid input values;\n"
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "khwilson (via GitHub)" <gi...@apache.org>.
khwilson commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1871972349
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -273,6 +328,29 @@ struct Atan {
}
};
+struct Atanh {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
Review Comment:
Oh totally! I honestly think both implementations can be rigorously justified and defended.
For instance, I could *define* atanh as the inverse of tanh. And I could *define* f(oo) = lim_{x -> oo} f(x). And then atanh(1) is clearly oo.
Just a matter of which convention the library wants to follow 😀
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "khwilson (via GitHub)" <gi...@apache.org>.
khwilson commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1872077589
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc:
##########
@@ -2401,6 +2405,19 @@ TYPED_TEST(TestUnaryArithmeticFloating, TrigSin) {
this->AssertUnaryOpRaises(Sin, "[Inf, -Inf]", "domain error");
}
+TYPED_TEST(TestUnaryArithmeticFloating, TrigSinh) {
+ this->SetNansEqual(true);
+ auto sinh = [](const Datum& arg, ArithmeticOptions, ExecContext* ctx) {
+ return Sinh(arg, ctx);
+ };
+
+ this->SetNansEqual(true);
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "khwilson (via GitHub)" <gi...@apache.org>.
khwilson commented on PR #44630:
URL: https://github.com/apache/arrow/pull/44630#issuecomment-2520878509
Thanks for the reviews. I can:
* Fix up the Python docs;
* Update the C++ docs for all the trig functions to reflect their domains instead of just "invalid inputs."
* Sort the code as suggested.
Remaining question for reviewers. In C++, `atanh(1) = + infinity` (and sets a divide by zero error). In Python `atanh(1)` is a domain error.
Which behaviour should this library (which spans both C++ and Python) utilize?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "khwilson (via GitHub)" <gi...@apache.org>.
khwilson commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1872381724
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1221,6 +1317,16 @@ const FunctionDoc atan2_doc{"Compute the inverse tangent of y/x",
("The return value is in the range [-pi, pi]."),
{"y", "x"}};
+const FunctionDoc atanh_doc{"Compute the inverse hyperbolic tangent",
+ ("NaN is returned for invalid input values;\n"
+ "to raise an error instead, see \"atanh_checked\"."),
+ {"x"}};
Review Comment:
Chose slightly different wording, but same spriti.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "khwilson (via GitHub)" <gi...@apache.org>.
khwilson commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1872383079
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc:
##########
@@ -2476,6 +2541,19 @@ TYPED_TEST(TestBinaryArithmeticFloating, TrigAtan2) {
-M_PI_2, 0, M_PI));
}
+TYPED_TEST(TestUnaryArithmeticFloating, TrigAtanh) {
+ this->SetNansEqual(true);
+ this->AssertUnaryOp(Atanh, "[-Inf, Inf, -2, 2]", "[NaN, NaN, NaN, NaN]");
Review Comment:
That happened a few lines below. But now that they behave differently I've split out the test on lines 2546 and 2553.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1871937532
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -273,6 +328,29 @@ struct Atan {
}
};
+struct Atanh {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
Review Comment:
C++ docs also say `If the argument is ±1, a pole error occurs.` and `if the argument is ±1, ±∞ is returned and [FE_DIVBYZERO] is raised.`
They have different names for the errors (range/pole/FE_DIVBYZERO) but that doesn't change the fact that Mathematically, `atanh` isn't defined at the -1.0 and 1.0 points.
The function **converges to** $\pm\infty$ as $x$ approaches the extremes of the range which makes returning `-Inf/+Inf` more reasonable than returning `NaN` at these extremes, but it's not defined at these points per se.
$$\lim_{x \to 1.0}{tanh^{-1}(x)} = \infty$$
$$\lim_{x \to -1.0}{tanh^{-1}(x)} = -\infty$$
So I think you can drop my suggestion on the unchecked version (this one) and let the implementation return `-Inf/+Inf` instead of `NaN` at these extremes, but the `atanh_checked` variation should fail at `-1.0/+1.0` instead of returning the infinite values.
~Docs should be updated accordingly to describe this behavior.~
I updated my suggestions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on PR #44630:
URL: https://github.com/apache/arrow/pull/44630#issuecomment-2526330809
@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 `\n`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44376: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on code in PR #44630:
URL: https://github.com/apache/arrow/pull/44630#discussion_r1870572815
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -273,6 +328,29 @@ struct Atan {
}
};
+struct Atanh {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
Review Comment:
At val=1 and val=-1, isn't atanh Inf and -Inf, respectively?
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1212,6 +1298,16 @@ const FunctionDoc acos_checked_doc{"Compute the inverse cosine",
"to return NaN instead, see \"acos\"."),
{"x"}};
+const FunctionDoc acosh_doc{"Compute the inverse hyperbolic cosine",
+ ("NaN is returned for invalid input values;\n"
Review Comment:
What do you think about specifying the range that produces NaN instead of just saying "invalid input values"?
##########
cpp/src/arrow/engine/substrait/extension_set.cc:
##########
@@ -1207,7 +1208,13 @@ struct DefaultExtensionIdRegistry : ExtensionIdRegistryImpl {
{kSubstraitArithmeticFunctionsUri, "acos"},
{kSubstraitArithmeticFunctionsUri, "asin"},
{kSubstraitArithmeticFunctionsUri, "atan"},
- {kSubstraitArithmeticFunctionsUri, "atan2"}}) {
+ {kSubstraitArithmeticFunctionsUri, "atan2"},
Review Comment:
Thanks for catching that we can add these now, I probably would've missed it in review. No changes needed here.
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -273,6 +328,29 @@ struct Atan {
}
};
+struct Atanh {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::atanh(val);
+ }
+};
+
+struct AtanhChecked {
+ template <typename T, typename Arg0>
+ static enable_if_floating_value<Arg0, T> Call(KernelContext*, Arg0 val, Status* st) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ if (ARROW_PREDICT_FALSE((val < -1.0 || val > 1.0))) {
Review Comment:
same question as above about Inf/-Inf
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44376: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on PR #44630:
URL: https://github.com/apache/arrow/pull/44630#issuecomment-2519088827
@felipecrv do you have any time to give this a second review?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44376: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "khwilson (via GitHub)" <gi...@apache.org>.
khwilson commented on PR #44630:
URL: https://github.com/apache/arrow/pull/44630#issuecomment-2495541098
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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-44952: [C++][Python] Add Hyperbolic Trig functions [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #44630:
URL: https://github.com/apache/arrow/pull/44630#issuecomment-2579061406
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 104b0406ab7e6bfc296f64649a6a71314436f06e.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/35348259137) has more details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org