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/21 17:15:39 UTC
[GitHub] [arrow] lidavidm opened a new pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
lidavidm opened a new pull request #10567:
URL: https://github.com/apache/arrow/pull/10567
Adds ln, log10, and log2. We could add a log1e and/or a logN if useful (probably not?)
Has some code from/will conflict with #10544.
--
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
[GitHub] [arrow] pitrou commented on a change in pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#discussion_r663994305
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -686,6 +686,118 @@ struct Atan2 {
}
};
+struct LogNatural {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log(arg);
+ }
+};
+
+struct LogNaturalChecked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ *st = Status::Invalid("divide by zero");
+ return arg;
+ } else if (arg < 0.0) {
+ *st = Status::Invalid("domain error");
+ return arg;
+ }
+ return std::log(arg);
+ }
+};
+
+struct Log10 {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log10(arg);
+ }
+};
+
+struct Log10Checked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0) {
+ *st = Status::Invalid("divide by zero");
+ return arg;
+ } else if (arg < 0) {
+ *st = Status::Invalid("domain error");
+ return arg;
+ }
+ return std::log10(arg);
+ }
+};
+
+struct Log2 {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log2(arg);
+ }
+};
+
+struct Log2Checked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ *st = Status::Invalid("divide by zero");
+ return arg;
+ } else if (arg < 0.0) {
+ *st = Status::Invalid("domain error");
+ return arg;
+ }
+ return std::log2(arg);
+ }
+};
Review comment:
Feel free to do it or not, in any case. This can also be merged as-is :-)
--
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
[GitHub] [arrow] edponce commented on a change in pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#discussion_r655907205
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -1511,5 +1517,55 @@ TEST(TestBinaryArithmeticDecimal, Divide) {
}
}
+TYPED_TEST(TestUnaryArithmeticFloating, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1, 2.7182818284590452354]"),
+ ArrayFromJSON(ty, "[0, 1]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"), ArrayFromJSON(ty, "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"), ArrayFromJSON(ty, "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticSigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(), "[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticUnsigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(), "[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+}
+
Review comment:
In valid test cases, no need to use `ty` (aka `this->type_singleton()`) with `ArrayFromJSON` because that is the default type.
Moreover, PR #10395 extends the unary scalar arithmetic test class to support combinations of JSON/Array inputs which will allow you to further simplify the test statements as follows:
* For integer inputs: `AssertUnaryOp(Ln, "[1]", ArrayFromJSON(float64(), "[0]"));`
* For floating point inputs: `AssertUnaryOp(Log10, "[1, 10]", "[0, 1]");`
--
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
[GitHub] [arrow] lidavidm commented on pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#issuecomment-872989061
I've extended the test cases and fixed the CI failure (by avoiding bouncing through RapidJSON for the failing assertions).
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#issuecomment-865207432
https://issues.apache.org/jira/browse/ARROW-13096
--
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
[GitHub] [arrow] edponce commented on a change in pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#discussion_r655908488
##########
File path: docs/source/cpp/compute.rst
##########
@@ -274,6 +274,18 @@ an ``Invalid`` :class:`Status` when overflow is detected.
+--------------------------+------------+--------------------+---------------------+
| divide_checked | Binary | Numeric | Numeric (1) |
+--------------------------+------------+--------------------+---------------------+
+| ln | Unary | Numeric | Numeric |
++--------------------------+------------+--------------------+---------------------+
+| ln_checked | Unary | Numeric | Numeric |
++--------------------------+------------+--------------------+---------------------+
+| log10 | Unary | Numeric | Numeric |
++--------------------------+------------+--------------------+---------------------+
+| log10_checked | Unary | Numeric | Numeric |
++--------------------------+------------+--------------------+---------------------+
+| log2 | Unary | Numeric | Numeric |
++--------------------------+------------+--------------------+---------------------+
+| log2_checked | Unary | Numeric | Numeric |
++--------------------------+------------+--------------------+---------------------+
Review comment:
I would suggest to add a note stating that these functions return `float64` value for integral inputs and same type as input for floating-point inputs.
--
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
[GitHub] [arrow] edponce commented on a change in pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#discussion_r655920884
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -485,6 +644,37 @@ ArrayKernelExec ArithmeticExecFromOp(detail::GetTypeId get_id) {
}
}
+// For kernels that always return floating results
+template <template <typename... Args> class KernelGenerator, typename Op>
+ArrayKernelExec IntToDoubleExecFromOp(detail::GetTypeId get_id) {
Review comment:
The variety of generator dispatchers available in the compute kernel are not consistent with their names. At some point we should rename them consistently, but that is out-of-scope for this PR. Nevertheless, I suggest to change the name of this generator dispatcher to `GenerateUnaryArithmeticFloatOutType`. This is simply a suggestion, we can rename later.
--
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
[GitHub] [arrow] cyb70289 closed pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
cyb70289 closed pull request #10567:
URL: https://github.com/apache/arrow/pull/10567
--
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
[GitHub] [arrow] pitrou commented on a change in pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#discussion_r663993579
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -686,6 +686,118 @@ struct Atan2 {
}
};
+struct LogNatural {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log(arg);
+ }
+};
+
+struct LogNaturalChecked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ *st = Status::Invalid("divide by zero");
+ return arg;
+ } else if (arg < 0.0) {
+ *st = Status::Invalid("domain error");
+ return arg;
+ }
+ return std::log(arg);
+ }
+};
+
+struct Log10 {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log10(arg);
+ }
+};
+
+struct Log10Checked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0) {
+ *st = Status::Invalid("divide by zero");
+ return arg;
+ } else if (arg < 0) {
+ *st = Status::Invalid("domain error");
+ return arg;
+ }
+ return std::log10(arg);
+ }
+};
+
+struct Log2 {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log2(arg);
+ }
+};
+
+struct Log2Checked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ *st = Status::Invalid("divide by zero");
+ return arg;
+ } else if (arg < 0.0) {
+ *st = Status::Invalid("domain error");
+ return arg;
+ }
+ return std::log2(arg);
+ }
+};
Review comment:
Perhaps, or on a struct defining those two functions.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -686,6 +686,118 @@ struct Atan2 {
}
};
+struct LogNatural {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log(arg);
+ }
+};
+
+struct LogNaturalChecked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ *st = Status::Invalid("divide by zero");
+ return arg;
+ } else if (arg < 0.0) {
+ *st = Status::Invalid("domain error");
+ return arg;
+ }
+ return std::log(arg);
+ }
+};
+
+struct Log10 {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log10(arg);
+ }
+};
+
+struct Log10Checked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0) {
+ *st = Status::Invalid("divide by zero");
+ return arg;
+ } else if (arg < 0) {
+ *st = Status::Invalid("domain error");
+ return arg;
+ }
+ return std::log10(arg);
+ }
+};
+
+struct Log2 {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log2(arg);
+ }
+};
+
+struct Log2Checked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ *st = Status::Invalid("divide by zero");
+ return arg;
+ } else if (arg < 0.0) {
+ *st = Status::Invalid("domain error");
+ return arg;
+ }
+ return std::log2(arg);
+ }
+};
Review comment:
Feel free to do it or not, in any case. This can also be merged as-is :-)
--
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
[GitHub] [arrow] edponce commented on a change in pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#discussion_r655907205
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -1511,5 +1517,55 @@ TEST(TestBinaryArithmeticDecimal, Divide) {
}
}
+TYPED_TEST(TestUnaryArithmeticFloating, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1, 2.7182818284590452354]"),
+ ArrayFromJSON(ty, "[0, 1]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"), ArrayFromJSON(ty, "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"), ArrayFromJSON(ty, "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticSigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(), "[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticUnsigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(), "[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+}
+
Review comment:
In valid test cases, no need to use `ty` (aka `this->type_singleton()`) with `ArrayFromJSON` because that is the default type. Also, no need to use the `this` keyword for the class methods.
Moreover, PR #10395 extends the unary scalar arithmetic test class to support combinations of JSON/Array inputs which will allow you to further simplify the test statements as follows:
* For integer inputs: `AssertUnaryOp(Ln, "[1]", ArrayFromJSON(float64(), "[0]"));`
* For floating point inputs: `AssertUnaryOp(Log10, "[1, 10]", "[0, 1]");`
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -1511,5 +1517,55 @@ TEST(TestBinaryArithmeticDecimal, Divide) {
}
}
+TYPED_TEST(TestUnaryArithmeticFloating, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1, 2.7182818284590452354]"),
+ ArrayFromJSON(ty, "[0, 1]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"), ArrayFromJSON(ty, "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"), ArrayFromJSON(ty, "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticSigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(), "[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticUnsigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(), "[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+}
+
Review comment:
Add test cases with `Inf` and `NaN` inputs.
##########
File path: docs/source/cpp/compute.rst
##########
@@ -274,6 +274,18 @@ an ``Invalid`` :class:`Status` when overflow is detected.
+--------------------------+------------+--------------------+---------------------+
| divide_checked | Binary | Numeric | Numeric (1) |
+--------------------------+------------+--------------------+---------------------+
+| ln | Unary | Numeric | Numeric |
++--------------------------+------------+--------------------+---------------------+
+| ln_checked | Unary | Numeric | Numeric |
++--------------------------+------------+--------------------+---------------------+
+| log10 | Unary | Numeric | Numeric |
++--------------------------+------------+--------------------+---------------------+
+| log10_checked | Unary | Numeric | Numeric |
++--------------------------+------------+--------------------+---------------------+
+| log2 | Unary | Numeric | Numeric |
++--------------------------+------------+--------------------+---------------------+
+| log2_checked | Unary | Numeric | Numeric |
++--------------------------+------------+--------------------+---------------------+
Review comment:
I would suggest to add a note stating that these functions return `float64` value for integral inputs and same type as input for floating-point inputs.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -1511,5 +1517,55 @@ TEST(TestBinaryArithmeticDecimal, Divide) {
}
}
+TYPED_TEST(TestUnaryArithmeticFloating, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1, 2.7182818284590452354]"),
+ ArrayFromJSON(ty, "[0, 1]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"), ArrayFromJSON(ty, "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"), ArrayFromJSON(ty, "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticSigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(), "[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticUnsigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(), "[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+}
+
Review comment:
Add test cases with `Inf`, `NaN`, `null`, `min`, `max` inputs.
For min/max you can refer to the tests of [`AbsoluteValue`](https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc#L1111-L1147).
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -1511,5 +1517,55 @@ TEST(TestBinaryArithmeticDecimal, Divide) {
}
}
+TYPED_TEST(TestUnaryArithmeticFloating, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1, 2.7182818284590452354]"),
+ ArrayFromJSON(ty, "[0, 1]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"), ArrayFromJSON(ty, "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"), ArrayFromJSON(ty, "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticSigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(), "[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticUnsigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(), "[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+}
+
Review comment:
In valid test cases, no need to use `ty` (aka `this->type_singleton()`) with `ArrayFromJSON` because that is the default type.
Moreover, PR #10395 extends the unary scalar arithmetic test class to support combinations of JSON/Array inputs which will allow you to further simplify the test statements as follows:
* For integer inputs: `AssertUnaryOp(Ln, "[1]", ArrayFromJSON(float64(), "[0]"));`
* For floating point inputs: `AssertUnaryOp(Log10, "[1, 10]", "[0, 1]");`
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -734,6 +924,19 @@ std::shared_ptr<ScalarFunction> MakeUnarySignedArithmeticFunctionNotNull(
return func;
}
+// Integer arguments return double values
+template <typename Op>
+std::shared_ptr<ScalarFunction> MakeUnaryIntToDoubleNotNull(std::string name,
+ const FunctionDoc* doc) {
+ auto func = std::make_shared<ArithmeticFunction>(name, Arity::Unary(), doc);
+ for (const auto& ty : NumericTypes()) {
+ auto output = is_integer(ty->id()) ? float64() : ty;
+ auto exec = IntToDoubleExecFromOp<ScalarUnaryNotNull, Op>(ty);
+ DCHECK_OK(func->AddKernel({ty}, output, exec));
+ }
+ return func;
+}
+
Review comment:
I suggest to change the function name to `MakeUnaryArithmeticFunctionFloatOutTypeNotNull`.
Also, the `_checked` variants use the `ScalarUnaryNotNull` kernel exec generator but the regular variants use `ScalarUnary`. Need to add `MakeUnaryArithmeticFunctionFloatOutType` with same logic but using `ScalarUnary`.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -485,6 +644,37 @@ ArrayKernelExec ArithmeticExecFromOp(detail::GetTypeId get_id) {
}
}
+// For kernels that always return floating results
+template <template <typename... Args> class KernelGenerator, typename Op>
+ArrayKernelExec IntToDoubleExecFromOp(detail::GetTypeId get_id) {
Review comment:
The variety of generator dispatchers available in the compute kernel are not consistent with their names. At some point we should rename them consistently, but that is out-of-scope for this PR. Nevertheless, I suggest to change the name of this generator dispatcher to `GenerateUnaryArithmeticFloatOutType`. This is simply a suggestion, we can rename later.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -485,6 +644,37 @@ ArrayKernelExec ArithmeticExecFromOp(detail::GetTypeId get_id) {
}
}
+// For kernels that always return floating results
+template <template <typename... Args> class KernelGenerator, typename Op>
+ArrayKernelExec IntToDoubleExecFromOp(detail::GetTypeId get_id) {
Review comment:
*For context:* There are a variety of generator dispatchers in the compute layer and their names are inconsistent (this is indirectly related to ARROW-9161). There has been previous work in [renaming them for consistency](https://github.com/apache/arrow/pull/7461#issuecomment-645623392) but looking at the codebase, we will need another pass.
I suggest to change the name `IntToDoubleExecFromOp` to `GenerateArithmeticWithFloatOutType`.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -734,6 +924,19 @@ std::shared_ptr<ScalarFunction> MakeUnarySignedArithmeticFunctionNotNull(
return func;
}
+// Integer arguments return double values
+template <typename Op>
+std::shared_ptr<ScalarFunction> MakeUnaryIntToDoubleNotNull(std::string name,
+ const FunctionDoc* doc) {
+ auto func = std::make_shared<ArithmeticFunction>(name, Arity::Unary(), doc);
+ for (const auto& ty : NumericTypes()) {
+ auto output = is_integer(ty->id()) ? float64() : ty;
+ auto exec = IntToDoubleExecFromOp<ScalarUnaryNotNull, Op>(ty);
+ DCHECK_OK(func->AddKernel({ty}, output, exec));
+ }
+ return func;
+}
+
Review comment:
I suggest to change the function name to `MakeUnaryArithmeticFunctionWithFloatOutTypeNotNull`.
Also, the `_checked` variants use the `ScalarUnaryNotNull` kernel exec generator but the regular variants use `ScalarUnary`. Need to add `MakeUnaryArithmeticFunctionWithFloatOutType` with same logic but using `ScalarUnary`.
--
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
[GitHub] [arrow] edponce commented on a change in pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#discussion_r655907796
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -1511,5 +1517,55 @@ TEST(TestBinaryArithmeticDecimal, Divide) {
}
}
+TYPED_TEST(TestUnaryArithmeticFloating, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1, 2.7182818284590452354]"),
+ ArrayFromJSON(ty, "[0, 1]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"), ArrayFromJSON(ty, "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"), ArrayFromJSON(ty, "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticSigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(), "[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticUnsigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(), "[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+}
+
Review comment:
Add test cases with `Inf` and `NaN` inputs.
--
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
[GitHub] [arrow] pitrou commented on pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#issuecomment-872398480
Hmm, there's a genuine CI failure here:
https://github.com/apache/arrow/pull/10567/checks?check_run_id=2954826698
--
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
[GitHub] [arrow] edponce commented on a change in pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#discussion_r655907796
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -1511,5 +1517,55 @@ TEST(TestBinaryArithmeticDecimal, Divide) {
}
}
+TYPED_TEST(TestUnaryArithmeticFloating, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1, 2.7182818284590452354]"),
+ ArrayFromJSON(ty, "[0, 1]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"), ArrayFromJSON(ty, "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"), ArrayFromJSON(ty, "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticSigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(), "[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticUnsigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(), "[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+}
+
Review comment:
Add test cases with `Inf`, `NaN`, `null`, `min`, `max` inputs.
For min/max you can refer to the tests of [`AbsoluteValue`](https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc#L1111-L1147).
--
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
[GitHub] [arrow] edponce commented on a change in pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#discussion_r655907205
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -1511,5 +1517,55 @@ TEST(TestBinaryArithmeticDecimal, Divide) {
}
}
+TYPED_TEST(TestUnaryArithmeticFloating, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1, 2.7182818284590452354]"),
+ ArrayFromJSON(ty, "[0, 1]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"), ArrayFromJSON(ty, "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"), ArrayFromJSON(ty, "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticSigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(), "[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+}
+
+TYPED_TEST(TestUnaryArithmeticUnsigned, Log) {
+ auto ty = this->type_singleton();
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1]"), ArrayFromJSON(float64(), "[0]"));
+ this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2]"),
+ ArrayFromJSON(float64(), "[0, 1]"));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+}
+
Review comment:
In valid test cases, no need to use `ty` (aka `this->type_singleton()`) with `ArrayFromJSON` because that is the default type. Also, no need to use the `this` keyword for the class methods.
Moreover, PR #10395 extends the unary scalar arithmetic test class to support combinations of JSON/Array inputs which will allow you to further simplify the test statements as follows:
* For integer inputs: `AssertUnaryOp(Ln, "[1]", ArrayFromJSON(float64(), "[0]"));`
* For floating point inputs: `AssertUnaryOp(Log10, "[1, 10]", "[0, 1]");`
--
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
[GitHub] [arrow] edponce commented on a change in pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#discussion_r655927814
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -485,6 +644,37 @@ ArrayKernelExec ArithmeticExecFromOp(detail::GetTypeId get_id) {
}
}
+// For kernels that always return floating results
+template <template <typename... Args> class KernelGenerator, typename Op>
+ArrayKernelExec IntToDoubleExecFromOp(detail::GetTypeId get_id) {
Review comment:
*For context:* There are a variety of generator dispatchers in the compute layer and their names are inconsistent (this is indirectly related to ARROW-9161). There has been previous work in [renaming them for consistency](https://github.com/apache/arrow/pull/7461#issuecomment-645623392) but looking at the codebase, we will need another pass.
I suggest to change the name `IntToDoubleExecFromOp` to `GenerateArithmeticWithFloatOutType`.
--
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
[GitHub] [arrow] lidavidm commented on a change in pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#discussion_r664511440
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -686,6 +686,118 @@ struct Atan2 {
}
};
+struct LogNatural {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log(arg);
+ }
+};
+
+struct LogNaturalChecked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ *st = Status::Invalid("divide by zero");
+ return arg;
+ } else if (arg < 0.0) {
+ *st = Status::Invalid("domain error");
+ return arg;
+ }
+ return std::log(arg);
+ }
+};
+
+struct Log10 {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log10(arg);
+ }
+};
+
+struct Log10Checked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0) {
+ *st = Status::Invalid("divide by zero");
+ return arg;
+ } else if (arg < 0) {
+ *st = Status::Invalid("domain error");
+ return arg;
+ }
+ return std::log10(arg);
+ }
+};
+
+struct Log2 {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log2(arg);
+ }
+};
+
+struct Log2Checked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ *st = Status::Invalid("divide by zero");
+ return arg;
+ } else if (arg < 0.0) {
+ *st = Status::Invalid("domain error");
+ return arg;
+ }
+ return std::log2(arg);
+ }
+};
Review comment:
I don't think it'll save us very much here (and our arithmetic functions are all written out instead of being templated).
--
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
[GitHub] [arrow] edponce commented on a change in pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#discussion_r655918970
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -734,6 +924,19 @@ std::shared_ptr<ScalarFunction> MakeUnarySignedArithmeticFunctionNotNull(
return func;
}
+// Integer arguments return double values
+template <typename Op>
+std::shared_ptr<ScalarFunction> MakeUnaryIntToDoubleNotNull(std::string name,
+ const FunctionDoc* doc) {
+ auto func = std::make_shared<ArithmeticFunction>(name, Arity::Unary(), doc);
+ for (const auto& ty : NumericTypes()) {
+ auto output = is_integer(ty->id()) ? float64() : ty;
+ auto exec = IntToDoubleExecFromOp<ScalarUnaryNotNull, Op>(ty);
+ DCHECK_OK(func->AddKernel({ty}, output, exec));
+ }
+ return func;
+}
+
Review comment:
I suggest to change the function name to `MakeUnaryArithmeticFunctionWithFloatOutTypeNotNull`.
Also, the `_checked` variants use the `ScalarUnaryNotNull` kernel exec generator but the regular variants use `ScalarUnary`. Need to add `MakeUnaryArithmeticFunctionWithFloatOutType` with same logic but using `ScalarUnary`.
--
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
[GitHub] [arrow] lidavidm commented on pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#issuecomment-866005033
@edponce thanks for the feedback! Since I also used these in #10544 I've made the changes there - if they look OK, then I'll backport them here. I had to do a little refactoring since ScalarBinary(EqualTypes) didn't specify a template parameter list and that threw off the inference.
--
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
[GitHub] [arrow] lidavidm commented on pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#issuecomment-871365421
I will rebase this once #10544 merges since there'll be more conflicts there anyways.
--
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
[GitHub] [arrow] lidavidm commented on a change in pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#discussion_r662455707
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -686,6 +686,118 @@ struct Atan2 {
}
};
+struct LogNatural {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log(arg);
+ }
+};
+
+struct LogNaturalChecked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ *st = Status::Invalid("divide by zero");
+ return arg;
+ } else if (arg < 0.0) {
+ *st = Status::Invalid("domain error");
+ return arg;
+ }
+ return std::log(arg);
+ }
+};
+
+struct Log10 {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log10(arg);
+ }
+};
+
+struct Log10Checked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0) {
+ *st = Status::Invalid("divide by zero");
+ return arg;
+ } else if (arg < 0) {
+ *st = Status::Invalid("domain error");
+ return arg;
+ }
+ return std::log10(arg);
+ }
+};
+
+struct Log2 {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log2(arg);
+ }
+};
+
+struct Log2Checked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ *st = Status::Invalid("divide by zero");
+ return arg;
+ } else if (arg < 0.0) {
+ *st = Status::Invalid("domain error");
+ return arg;
+ }
+ return std::log2(arg);
+ }
+};
Review comment:
I could make it templated on two functions (one for float, one for double)?
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#issuecomment-865207432
https://issues.apache.org/jira/browse/ARROW-13096
--
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
[GitHub] [arrow] edponce commented on a change in pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#discussion_r655918970
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -734,6 +924,19 @@ std::shared_ptr<ScalarFunction> MakeUnarySignedArithmeticFunctionNotNull(
return func;
}
+// Integer arguments return double values
+template <typename Op>
+std::shared_ptr<ScalarFunction> MakeUnaryIntToDoubleNotNull(std::string name,
+ const FunctionDoc* doc) {
+ auto func = std::make_shared<ArithmeticFunction>(name, Arity::Unary(), doc);
+ for (const auto& ty : NumericTypes()) {
+ auto output = is_integer(ty->id()) ? float64() : ty;
+ auto exec = IntToDoubleExecFromOp<ScalarUnaryNotNull, Op>(ty);
+ DCHECK_OK(func->AddKernel({ty}, output, exec));
+ }
+ return func;
+}
+
Review comment:
I suggest to change the function name to `MakeUnaryArithmeticFunctionFloatOutTypeNotNull`.
Also, the `_checked` variants use the `ScalarUnaryNotNull` kernel exec generator but the regular variants use `ScalarUnary`. Need to add `MakeUnaryArithmeticFunctionFloatOutType` with same logic but using `ScalarUnary`.
--
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
[GitHub] [arrow] pitrou commented on a change in pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#discussion_r662440835
##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -396,6 +396,52 @@ Result<Datum> Atan(const Datum& arg, ExecContext* ctx = NULLPTR);
ARROW_EXPORT
Result<Datum> Atan2(const Datum& y, const Datum& x, ExecContext* ctx = NULLPTR);
+/// \brief Get the natural log of a value. Array values can be of arbitrary
+/// length. If argument is null the result will be null.
+///
+/// \param[in] arg the value transformed
Review comment:
Why "transformed"?
##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -396,6 +396,52 @@ Result<Datum> Atan(const Datum& arg, ExecContext* ctx = NULLPTR);
ARROW_EXPORT
Result<Datum> Atan2(const Datum& y, const Datum& x, ExecContext* ctx = NULLPTR);
+/// \brief Get the natural log of a value. Array values can be of arbitrary
+/// length. If argument is null the result will be null.
Review comment:
I'm not sure that "Array values can be of arbitrary length" is a useful mention. It's normally true of all scalar (elemen-wise) functions.
Also, can we keep the `\brief` sentence a single one-liner, and put the description after a newline?
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -686,6 +686,118 @@ struct Atan2 {
}
};
+struct LogNatural {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log(arg);
+ }
+};
+
+struct LogNaturalChecked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ *st = Status::Invalid("divide by zero");
+ return arg;
+ } else if (arg < 0.0) {
+ *st = Status::Invalid("domain error");
Review comment:
Perhaps something more precise, e.g. "logarithm of negative number"?
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -686,6 +686,118 @@ struct Atan2 {
}
};
+struct LogNatural {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log(arg);
+ }
+};
+
+struct LogNaturalChecked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ *st = Status::Invalid("divide by zero");
+ return arg;
+ } else if (arg < 0.0) {
+ *st = Status::Invalid("domain error");
+ return arg;
+ }
+ return std::log(arg);
+ }
+};
+
+struct Log10 {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log10(arg);
+ }
+};
+
+struct Log10Checked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0) {
+ *st = Status::Invalid("divide by zero");
+ return arg;
+ } else if (arg < 0) {
+ *st = Status::Invalid("domain error");
+ return arg;
+ }
+ return std::log10(arg);
+ }
+};
+
+struct Log2 {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log2(arg);
+ }
+};
+
+struct Log2Checked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ *st = Status::Invalid("divide by zero");
+ return arg;
+ } else if (arg < 0.0) {
+ *st = Status::Invalid("domain error");
+ return arg;
+ }
+ return std::log2(arg);
+ }
+};
Review comment:
I don't know if that's worth it, but these three kernels have very similar implementations, maybe something could be shared. Or perhaps that's pointless generalization.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -686,6 +686,118 @@ struct Atan2 {
}
};
+struct LogNatural {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log(arg);
+ }
+};
+
+struct LogNaturalChecked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ *st = Status::Invalid("divide by zero");
Review comment:
I don't know if that's the best error message. Perhaps "logarithm of zero"?
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -1295,6 +1407,60 @@ const FunctionDoc atan2_doc{
"Compute the inverse tangent using argument signs to determine the quadrant",
("Integer arguments return double values."),
{"y", "x"}};
+
+const FunctionDoc ln_doc{
+ "Take natural log of arguments element-wise",
Review comment:
We use "Compute" above.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -1821,5 +1821,57 @@ TYPED_TEST(TestBinaryArithmeticFloating, TrigAtan2) {
-M_PI_2, 0, M_PI));
}
+TYPED_TEST(TestUnaryArithmeticFloating, Log) {
+ using CType = typename TestFixture::CType;
+ auto ty = this->type_singleton();
+ this->SetNansEqual(true);
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertUnaryOp(Ln, "[1, 2.7182818284590452354, null, NaN, Inf]",
+ "[0, 1, null, NaN, Inf]");
+ // N.B. min() for float types is smallest normal number > 0
+ this->AssertUnaryOp(Ln, std::numeric_limits<CType>::min(),
+ std::log(std::numeric_limits<CType>::min()));
+ this->AssertUnaryOp(Ln, std::numeric_limits<CType>::max(),
+ std::log(std::numeric_limits<CType>::max()));
+ this->AssertUnaryOp(Log10, "[1, 10, null, NaN, Inf]", "[0, 1, null, NaN, Inf]");
+ this->AssertUnaryOp(Log10, std::numeric_limits<CType>::min(),
+ std::log10(std::numeric_limits<CType>::min()));
+ this->AssertUnaryOp(Log10, std::numeric_limits<CType>::max(),
+ std::log10(std::numeric_limits<CType>::max()));
+ this->AssertUnaryOp(Log2, "[1, 2, null, NaN, Inf]", "[0, 1, null, NaN, Inf]");
+ this->AssertUnaryOp(Log2, std::numeric_limits<CType>::min(),
+ std::log2(std::numeric_limits<CType>::min()));
+ this->AssertUnaryOp(Log2, std::numeric_limits<CType>::max(),
+ std::log2(std::numeric_limits<CType>::max()));
+ this->AssertUnaryOp(Log1p, "[0, 1.7182818284590452354, null, NaN, Inf]",
+ "[0, 1, null, NaN, Inf]");
+ this->AssertUnaryOp(Log1p, std::numeric_limits<CType>::min(),
+ std::log1p(std::numeric_limits<CType>::min()));
+ this->AssertUnaryOp(Log1p, std::numeric_limits<CType>::max(),
+ std::log1p(std::numeric_limits<CType>::max()));
+ }
+ this->AssertUnaryOpRaises(Ln, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Ln, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Ln, "[-Inf]", "domain error");
+ this->AssertUnaryOpRaises(Ln, MakeArray(std::numeric_limits<CType>::lowest()),
+ "domain error");
+ this->AssertUnaryOpRaises(Log10, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log10, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log10, "[-Inf]", "domain error");
+ this->AssertUnaryOpRaises(Log10, MakeArray(std::numeric_limits<CType>::lowest()),
+ "domain error");
+ this->AssertUnaryOpRaises(Log2, "[0]", "divide by zero");
+ this->AssertUnaryOpRaises(Log2, "[-1]", "domain error");
+ this->AssertUnaryOpRaises(Log2, "[-Inf]", "domain error");
+ this->AssertUnaryOpRaises(Log2, MakeArray(std::numeric_limits<CType>::lowest()),
+ "domain error");
+ this->AssertUnaryOpRaises(Log1p, "[-1]", "divide by zero");
+ this->AssertUnaryOpRaises(Log1p, "[-2]", "domain error");
+ this->AssertUnaryOpRaises(Log1p, "[-Inf]", "domain error");
+ this->AssertUnaryOpRaises(Log1p, MakeArray(std::numeric_limits<CType>::lowest()),
+ "domain error");
Review comment:
Can we check the error cases for the non-checked variants as well?
--
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
[GitHub] [arrow] pitrou commented on a change in pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10567:
URL: https://github.com/apache/arrow/pull/10567#discussion_r663993579
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -686,6 +686,118 @@ struct Atan2 {
}
};
+struct LogNatural {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log(arg);
+ }
+};
+
+struct LogNaturalChecked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ *st = Status::Invalid("divide by zero");
+ return arg;
+ } else if (arg < 0.0) {
+ *st = Status::Invalid("domain error");
+ return arg;
+ }
+ return std::log(arg);
+ }
+};
+
+struct Log10 {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log10(arg);
+ }
+};
+
+struct Log10Checked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0) {
+ *st = Status::Invalid("divide by zero");
+ return arg;
+ } else if (arg < 0) {
+ *st = Status::Invalid("domain error");
+ return arg;
+ }
+ return std::log10(arg);
+ }
+};
+
+struct Log2 {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status*) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ return -std::numeric_limits<T>::infinity();
+ } else if (arg < 0.0) {
+ return std::numeric_limits<T>::quiet_NaN();
+ }
+ return std::log2(arg);
+ }
+};
+
+struct Log2Checked {
+ template <typename T, typename Arg>
+ static enable_if_floating_point<Arg, T> Call(KernelContext*, Arg arg, Status* st) {
+ static_assert(std::is_same<T, Arg>::value, "");
+ if (arg == 0.0) {
+ *st = Status::Invalid("divide by zero");
+ return arg;
+ } else if (arg < 0.0) {
+ *st = Status::Invalid("domain error");
+ return arg;
+ }
+ return std::log2(arg);
+ }
+};
Review comment:
Perhaps, or on a struct defining those two functions.
--
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