You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/06/18 21:13:32 UTC
[GitHub] [arrow] edponce commented on a change in pull request #10544: ARROW-13095: [C++] Implement trig compute functions
edponce commented on a change in pull request #10544:
URL: https://github.com/apache/arrow/pull/10544#discussion_r653780349
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -397,6 +397,136 @@ struct PowerChecked {
}
};
+// Stamp out a trig function for which +/-inf are domain errors
+#define TRIG_NO_INF(name, func) \
+ struct Trig##name { \
+ template <typename T, typename Arg0> \
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) { \
+ static_assert(std::is_same<T, double>::value, ""); \
+ return func(val); \
+ } \
+ template <typename T, typename Arg0> \
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) { \
+ static_assert(std::is_same<T, Arg0>::value, ""); \
+ return func(val); \
+ } \
+ }; \
+ struct Trig##name##Checked { \
+ template <typename T, typename Arg0> \
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) { \
+ static_assert(std::is_same<T, double>::value, ""); \
+ return func(val); \
+ } \
+ template <typename T, typename Arg0> \
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, \
+ Status* st) { \
+ static_assert(std::is_same<T, Arg0>::value, ""); \
+ if (ARROW_PREDICT_FALSE(std::isinf(val))) { \
+ *st = Status::Invalid("domain error"); \
+ return val; \
+ } \
+ return func(val); \
+ } \
+ };
+
+TRIG_NO_INF(Sin, std::sin);
+TRIG_NO_INF(Cos, std::cos);
+TRIG_NO_INF(Tan, std::tan);
+
+struct TrigAsin {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::asin(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ return std::asin(val);
+ }
+};
+
+struct TrigAsinChecked {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::asin(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<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) && !std::isnan(val))) {
+ *st = Status::Invalid("domain error");
+ return val;
+ }
+ return std::asin(val);
+ }
+};
+
+struct TrigAcos {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::acos(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ return std::acos(val);
+ }
+};
+
+struct TrigAcosChecked {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::acos(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<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) && !std::isnan(val))) {
+ *st = Status::Invalid("domain error");
+ return val;
+ }
+ return std::acos(val);
+ }
+};
+
+struct TrigAtan {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::atan(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ return std::atan(val);
+ }
+};
+
+struct TrigAtan2 {
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 y, Arg1 x, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ static_assert(std::is_same<Arg0, Arg1>::value, "");
+ return std::atan2(y, x);
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 y, Arg1 x, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ static_assert(std::is_same<Arg0, Arg1>::value, "");
+ return std::atan2(y, x);
+ }
+};
Review comment:
I have always thought of the `check_overflow` option to be applicable for both *underflow* and *overflow*. I do not see a benefit of having two distinct options. Regarding if you should include `ArithmeticOptions` just for the sakes of consistency, I would argue that should not be done. Why would we want to include an unused value? Also, it might confuse the client code if it thinks that the function options provide some functionality.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -397,6 +397,136 @@ struct PowerChecked {
}
};
+// Stamp out a trig function for which +/-inf are domain errors
+#define TRIG_NO_INF(name, func) \
+ struct Trig##name { \
+ template <typename T, typename Arg0> \
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) { \
+ static_assert(std::is_same<T, double>::value, ""); \
+ return func(val); \
+ } \
+ template <typename T, typename Arg0> \
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) { \
+ static_assert(std::is_same<T, Arg0>::value, ""); \
+ return func(val); \
+ } \
+ }; \
+ struct Trig##name##Checked { \
+ template <typename T, typename Arg0> \
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) { \
+ static_assert(std::is_same<T, double>::value, ""); \
+ return func(val); \
+ } \
+ template <typename T, typename Arg0> \
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, \
+ Status* st) { \
+ static_assert(std::is_same<T, Arg0>::value, ""); \
+ if (ARROW_PREDICT_FALSE(std::isinf(val))) { \
+ *st = Status::Invalid("domain error"); \
+ return val; \
+ } \
+ return func(val); \
+ } \
+ };
+
+TRIG_NO_INF(Sin, std::sin);
+TRIG_NO_INF(Cos, std::cos);
+TRIG_NO_INF(Tan, std::tan);
+
+struct TrigAsin {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::asin(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ return std::asin(val);
+ }
+};
+
+struct TrigAsinChecked {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::asin(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<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) && !std::isnan(val))) {
+ *st = Status::Invalid("domain error");
+ return val;
+ }
+ return std::asin(val);
+ }
+};
+
+struct TrigAcos {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::acos(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ return std::acos(val);
+ }
+};
+
+struct TrigAcosChecked {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::acos(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<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) && !std::isnan(val))) {
+ *st = Status::Invalid("domain error");
+ return val;
+ }
+ return std::acos(val);
+ }
+};
+
+struct TrigAtan {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::atan(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ return std::atan(val);
+ }
+};
+
+struct TrigAtan2 {
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 y, Arg1 x, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ static_assert(std::is_same<Arg0, Arg1>::value, "");
+ return std::atan2(y, x);
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 y, Arg1 x, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ static_assert(std::is_same<Arg0, Arg1>::value, "");
+ return std::atan2(y, x);
+ }
+};
Review comment:
C++ defines [atan2(0, 0)](https://www.cplusplus.com/reference/cmath/atan2/) as a domain error, the returned value is implementation-specific, so even if it returns 0 in our tests we should trigger a domain error as it maps to 0/0 expression.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -397,6 +397,136 @@ struct PowerChecked {
}
};
+// Stamp out a trig function for which +/-inf are domain errors
+#define TRIG_NO_INF(name, func) \
+ struct Trig##name { \
+ template <typename T, typename Arg0> \
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) { \
+ static_assert(std::is_same<T, double>::value, ""); \
+ return func(val); \
+ } \
+ template <typename T, typename Arg0> \
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) { \
+ static_assert(std::is_same<T, Arg0>::value, ""); \
+ return func(val); \
+ } \
+ }; \
+ struct Trig##name##Checked { \
+ template <typename T, typename Arg0> \
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) { \
+ static_assert(std::is_same<T, double>::value, ""); \
+ return func(val); \
+ } \
+ template <typename T, typename Arg0> \
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, \
+ Status* st) { \
+ static_assert(std::is_same<T, Arg0>::value, ""); \
+ if (ARROW_PREDICT_FALSE(std::isinf(val))) { \
+ *st = Status::Invalid("domain error"); \
+ return val; \
+ } \
+ return func(val); \
+ } \
+ };
+
+TRIG_NO_INF(Sin, std::sin);
+TRIG_NO_INF(Cos, std::cos);
+TRIG_NO_INF(Tan, std::tan);
+
+struct TrigAsin {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::asin(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ return std::asin(val);
+ }
+};
+
+struct TrigAsinChecked {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::asin(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<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) && !std::isnan(val))) {
+ *st = Status::Invalid("domain error");
+ return val;
+ }
+ return std::asin(val);
+ }
+};
+
+struct TrigAcos {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::acos(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ return std::acos(val);
+ }
+};
+
+struct TrigAcosChecked {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::acos(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<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) && !std::isnan(val))) {
+ *st = Status::Invalid("domain error");
+ return val;
+ }
+ return std::acos(val);
+ }
+};
+
+struct TrigAtan {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::atan(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ return std::atan(val);
+ }
+};
+
+struct TrigAtan2 {
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 y, Arg1 x, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ static_assert(std::is_same<Arg0, Arg1>::value, "");
+ return std::atan2(y, x);
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 y, Arg1 x, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ static_assert(std::is_same<Arg0, Arg1>::value, "");
+ return std::atan2(y, x);
+ }
+};
Review comment:
Thanks for the `atan2` link. Maybe a safer way to go (to prevent domain error for `atan2(0, 0)`) is to explicitly return 0 for this case. This way if IEEE 754 is not being conformed to, result is consistent and domain error is circumvented.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -397,6 +397,136 @@ struct PowerChecked {
}
};
+// Stamp out a trig function for which +/-inf are domain errors
+#define TRIG_NO_INF(name, func) \
+ struct Trig##name { \
+ template <typename T, typename Arg0> \
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) { \
+ static_assert(std::is_same<T, double>::value, ""); \
+ return func(val); \
+ } \
+ template <typename T, typename Arg0> \
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) { \
+ static_assert(std::is_same<T, Arg0>::value, ""); \
+ return func(val); \
+ } \
+ }; \
+ struct Trig##name##Checked { \
+ template <typename T, typename Arg0> \
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) { \
+ static_assert(std::is_same<T, double>::value, ""); \
+ return func(val); \
+ } \
+ template <typename T, typename Arg0> \
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, \
+ Status* st) { \
+ static_assert(std::is_same<T, Arg0>::value, ""); \
+ if (ARROW_PREDICT_FALSE(std::isinf(val))) { \
+ *st = Status::Invalid("domain error"); \
+ return val; \
+ } \
+ return func(val); \
+ } \
+ };
+
+TRIG_NO_INF(Sin, std::sin);
+TRIG_NO_INF(Cos, std::cos);
+TRIG_NO_INF(Tan, std::tan);
+
+struct TrigAsin {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::asin(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ return std::asin(val);
+ }
+};
+
+struct TrigAsinChecked {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::asin(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<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) && !std::isnan(val))) {
+ *st = Status::Invalid("domain error");
+ return val;
+ }
+ return std::asin(val);
+ }
+};
+
+struct TrigAcos {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::acos(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ return std::acos(val);
+ }
+};
+
+struct TrigAcosChecked {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::acos(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<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) && !std::isnan(val))) {
+ *st = Status::Invalid("domain error");
+ return val;
+ }
+ return std::acos(val);
+ }
+};
+
+struct TrigAtan {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::atan(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ return std::atan(val);
+ }
+};
+
+struct TrigAtan2 {
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 y, Arg1 x, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ static_assert(std::is_same<Arg0, Arg1>::value, "");
+ return std::atan2(y, x);
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 y, Arg1 x, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ static_assert(std::is_same<Arg0, Arg1>::value, "");
+ return std::atan2(y, x);
+ }
+};
Review comment:
Thanks for the `atan2` link. Maybe a safer way to go (to prevent domain error for `atan2(0, 0)`) is to explicitly return 0 for this case. This way if IEEE 754 is not being conformed to, result is consistent and domain error is circumvented. But what would be the performance effect of this comparison?
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -602,6 +787,80 @@ const FunctionDoc pow_checked_doc{
("An error is returned when integer to negative integer power is encountered,\n"
"or integer overflow is encountered."),
{"base", "exponent"}};
+
+const FunctionDoc sin_doc{"Computes the sine of the elements argument-wise",
+ ("Integer arguments return double values. "
+ "This function returns NaN on values outside its domain. "
+ "To raise an error instead, see sin_checked."),
+ {"x"}};
Review comment:
Based on all the other function docs, when a function is referenced in the text it is enclosed in double quotes. For example, change *sin_checked* to *\\"sin_checked\\"*.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -602,6 +787,80 @@ const FunctionDoc pow_checked_doc{
("An error is returned when integer to negative integer power is encountered,\n"
"or integer overflow is encountered."),
{"base", "exponent"}};
+
+const FunctionDoc sin_doc{"Computes the sine of the elements argument-wise",
+ ("Integer arguments return double values. "
+ "This function returns NaN on values outside its domain. "
+ "To raise an error instead, see sin_checked."),
+ {"x"}};
Review comment:
Based on all the other function docs, when a function is referenced in the text it is enclosed in double quotes. For example, change *sin_checked* to *\\"sin_checked\\"*. Similarly, for all the other cases.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -397,6 +397,136 @@ struct PowerChecked {
}
};
+// Stamp out a trig function for which +/-inf are domain errors
+#define TRIG_NO_INF(name, func) \
+ struct Trig##name { \
+ template <typename T, typename Arg0> \
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) { \
+ static_assert(std::is_same<T, double>::value, ""); \
+ return func(val); \
+ } \
+ template <typename T, typename Arg0> \
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) { \
+ static_assert(std::is_same<T, Arg0>::value, ""); \
+ return func(val); \
+ } \
+ }; \
+ struct Trig##name##Checked { \
+ template <typename T, typename Arg0> \
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) { \
+ static_assert(std::is_same<T, double>::value, ""); \
+ return func(val); \
+ } \
+ template <typename T, typename Arg0> \
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, \
+ Status* st) { \
+ static_assert(std::is_same<T, Arg0>::value, ""); \
+ if (ARROW_PREDICT_FALSE(std::isinf(val))) { \
+ *st = Status::Invalid("domain error"); \
+ return val; \
+ } \
+ return func(val); \
+ } \
+ };
+
+TRIG_NO_INF(Sin, std::sin);
+TRIG_NO_INF(Cos, std::cos);
+TRIG_NO_INF(Tan, std::tan);
+
+struct TrigAsin {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::asin(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ return std::asin(val);
+ }
+};
+
+struct TrigAsinChecked {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::asin(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<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) && !std::isnan(val))) {
+ *st = Status::Invalid("domain error");
+ return val;
+ }
+ return std::asin(val);
+ }
+};
+
+struct TrigAcos {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::acos(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ return std::acos(val);
+ }
+};
+
+struct TrigAcosChecked {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::acos(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<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) && !std::isnan(val))) {
+ *st = Status::Invalid("domain error");
+ return val;
+ }
+ return std::acos(val);
+ }
+};
+
+struct TrigAtan {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::atan(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ return std::atan(val);
+ }
+};
+
+struct TrigAtan2 {
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 y, Arg1 x, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ static_assert(std::is_same<Arg0, Arg1>::value, "");
+ return std::atan2(y, x);
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 y, Arg1 x, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ static_assert(std::is_same<Arg0, Arg1>::value, "");
+ return std::atan2(y, x);
+ }
+};
Review comment:
Well, probably there is no need to check for underflow, because it seems that for all trigonometric functions, C++ applies rounding to the underflow cases and returns a correct value.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -397,6 +397,136 @@ struct PowerChecked {
}
};
+// Stamp out a trig function for which +/-inf are domain errors
+#define TRIG_NO_INF(name, func) \
+ struct Trig##name { \
+ template <typename T, typename Arg0> \
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) { \
+ static_assert(std::is_same<T, double>::value, ""); \
+ return func(val); \
+ } \
+ template <typename T, typename Arg0> \
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) { \
+ static_assert(std::is_same<T, Arg0>::value, ""); \
+ return func(val); \
+ } \
+ }; \
+ struct Trig##name##Checked { \
+ template <typename T, typename Arg0> \
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) { \
+ static_assert(std::is_same<T, double>::value, ""); \
+ return func(val); \
+ } \
+ template <typename T, typename Arg0> \
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, \
+ Status* st) { \
+ static_assert(std::is_same<T, Arg0>::value, ""); \
+ if (ARROW_PREDICT_FALSE(std::isinf(val))) { \
+ *st = Status::Invalid("domain error"); \
+ return val; \
+ } \
+ return func(val); \
+ } \
+ };
+
+TRIG_NO_INF(Sin, std::sin);
+TRIG_NO_INF(Cos, std::cos);
+TRIG_NO_INF(Tan, std::tan);
+
+struct TrigAsin {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::asin(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ return std::asin(val);
+ }
+};
+
+struct TrigAsinChecked {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::asin(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<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) && !std::isnan(val))) {
+ *st = Status::Invalid("domain error");
+ return val;
+ }
+ return std::asin(val);
+ }
+};
+
+struct TrigAcos {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::acos(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ return std::acos(val);
+ }
+};
+
+struct TrigAcosChecked {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::acos(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<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) && !std::isnan(val))) {
+ *st = Status::Invalid("domain error");
+ return val;
+ }
+ return std::acos(val);
+ }
+};
+
+struct TrigAtan {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::atan(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ return std::atan(val);
+ }
+};
+
+struct TrigAtan2 {
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 y, Arg1 x, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ static_assert(std::is_same<Arg0, Arg1>::value, "");
+ return std::atan2(y, x);
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 y, Arg1 x, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ static_assert(std::is_same<Arg0, Arg1>::value, "");
+ return std::atan2(y, x);
+ }
+};
Review comment:
One last fix, `copy_sign` should also be applied to 0.0 value, `return std::copysign(static_cast<T>(0.0), y)`.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -397,6 +397,136 @@ struct PowerChecked {
}
};
+// Stamp out a trig function for which +/-inf are domain errors
+#define TRIG_NO_INF(name, func) \
+ struct Trig##name { \
+ template <typename T, typename Arg0> \
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) { \
+ static_assert(std::is_same<T, double>::value, ""); \
+ return func(val); \
+ } \
+ template <typename T, typename Arg0> \
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) { \
+ static_assert(std::is_same<T, Arg0>::value, ""); \
+ return func(val); \
+ } \
+ }; \
+ struct Trig##name##Checked { \
+ template <typename T, typename Arg0> \
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) { \
+ static_assert(std::is_same<T, double>::value, ""); \
+ return func(val); \
+ } \
+ template <typename T, typename Arg0> \
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, \
+ Status* st) { \
+ static_assert(std::is_same<T, Arg0>::value, ""); \
+ if (ARROW_PREDICT_FALSE(std::isinf(val))) { \
+ *st = Status::Invalid("domain error"); \
+ return val; \
+ } \
+ return func(val); \
+ } \
+ };
+
+TRIG_NO_INF(Sin, std::sin);
+TRIG_NO_INF(Cos, std::cos);
+TRIG_NO_INF(Tan, std::tan);
+
+struct TrigAsin {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::asin(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ return std::asin(val);
+ }
+};
+
+struct TrigAsinChecked {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::asin(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<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) && !std::isnan(val))) {
+ *st = Status::Invalid("domain error");
+ return val;
+ }
+ return std::asin(val);
+ }
+};
+
+struct TrigAcos {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::acos(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ return std::acos(val);
+ }
+};
+
+struct TrigAcosChecked {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::acos(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<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) && !std::isnan(val))) {
+ *st = Status::Invalid("domain error");
+ return val;
+ }
+ return std::acos(val);
+ }
+};
+
+struct TrigAtan {
+ template <typename T, typename Arg0>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ return std::atan(val);
+ }
+
+ template <typename T, typename Arg0>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 val, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ return std::atan(val);
+ }
+};
+
+struct TrigAtan2 {
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_integer<Arg0, T> Call(KernelContext*, Arg0 y, Arg1 x, Status*) {
+ static_assert(std::is_same<T, double>::value, "");
+ static_assert(std::is_same<Arg0, Arg1>::value, "");
+ return std::atan2(y, x);
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_floating_point<Arg0, T> Call(KernelContext*, Arg0 y, Arg1 x, Status*) {
+ static_assert(std::is_same<T, Arg0>::value, "");
+ static_assert(std::is_same<Arg0, Arg1>::value, "");
+ return std::atan2(y, x);
+ }
+};
Review comment:
One last fix, `std::copysign` should also be applied to 0.0 value, `return std::copysign(static_cast<T>(0.0), y)`.
##########
File path: cpp/src/arrow/compute/kernels/util_internal.h
##########
@@ -30,6 +30,24 @@ namespace arrow {
namespace compute {
namespace internal {
+// Used in some kernels and testing - not provided by default in MSVC
+// and _USE_MATH_DEFINES is not reliable with unity builds
+#ifndef M_E
+#define M_E 2.71828182845904523536
+#define M_LOG2E 1.44269504088896340736
+#define M_LOG10E 0.434294481903251827651
+#define M_LN2 0.693147180559945309417
+#define M_LN10 2.30258509299404568402
+#define M_PI 3.14159265358979323846
+#define M_PI_2 1.57079632679489661923
+#define M_PI_4 0.785398163397448309616
+#define M_1_PI 0.318309886183790671538
+#define M_2_PI 0.636619772367581343076
+#define M_2_SQRTPI 1.12837916709551257390
+#define M_SQRT2 1.41421356237309504880
+#define M_SQRT1_2 0.707106781186547524401
+#endif
+
Review comment:
I do not understand the issue with `_USE_MATH_DEFINES` and unity builds. These defines should be guarded with
```
#if defined(_USE_MATH_DEFINES) && !defined(_MATH_DEFINES_DEFINED)
#define _MATH_DEFINES_DEFINED
...
#endif
```
to prevent multiple definitions.
##########
File path: cpp/src/arrow/compute/kernels/util_internal.h
##########
@@ -30,6 +30,24 @@ namespace arrow {
namespace compute {
namespace internal {
+// Used in some kernels and testing - not provided by default in MSVC
+// and _USE_MATH_DEFINES is not reliable with unity builds
+#ifndef M_E
+#define M_E 2.71828182845904523536
+#define M_LOG2E 1.44269504088896340736
+#define M_LOG10E 0.434294481903251827651
+#define M_LN2 0.693147180559945309417
+#define M_LN10 2.30258509299404568402
+#define M_PI 3.14159265358979323846
+#define M_PI_2 1.57079632679489661923
+#define M_PI_4 0.785398163397448309616
+#define M_1_PI 0.318309886183790671538
+#define M_2_PI 0.636619772367581343076
+#define M_2_SQRTPI 1.12837916709551257390
+#define M_SQRT2 1.41421356237309504880
+#define M_SQRT1_2 0.707106781186547524401
+#endif
+
Review comment:
Ok, I see. You are guarding all the defines based on the existence of `M_E`. I think it would be "safer" to guard each macro independently. Safer in the sense that if at some point `M_E` is defined explicitly in another translation unit, then the remaining macros will be undefined.
##########
File path: cpp/src/arrow/compute/kernels/util_internal.h
##########
@@ -30,6 +30,24 @@ namespace arrow {
namespace compute {
namespace internal {
+// Used in some kernels and testing - not provided by default in MSVC
+// and _USE_MATH_DEFINES is not reliable with unity builds
+#ifndef M_E
+#define M_E 2.71828182845904523536
+#define M_LOG2E 1.44269504088896340736
+#define M_LOG10E 0.434294481903251827651
+#define M_LN2 0.693147180559945309417
+#define M_LN10 2.30258509299404568402
+#define M_PI 3.14159265358979323846
+#define M_PI_2 1.57079632679489661923
+#define M_PI_4 0.785398163397448309616
+#define M_1_PI 0.318309886183790671538
+#define M_2_PI 0.636619772367581343076
+#define M_2_SQRTPI 1.12837916709551257390
+#define M_SQRT2 1.41421356237309504880
+#define M_SQRT1_2 0.707106781186547524401
+#endif
+
Review comment:
Ok, I see. You are guarding all the defines based on the existence of `M_E`. I think it would be "safer" to guard each macro independently. Safer in the sense that if at some point only `M_E` is defined explicitly in another translation unit, then the remaining macros will be undefined. Alternatively, we can use `_USE_ARROW_MATH_DEFINES` and require it before including `util_internal.h`, but this seems overkill.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org