Re: [PR] GH-44376: [C++][Python] Add Hyperbolic Trig functions [arrow]

["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