[GitHub] [arrow] pitrou commented on a change in pull request #10567: ARROW-13096: [C++] Implement logarithm compute functions

[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