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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
