pitrou commented on code in PR #36932:
URL: https://github.com/apache/arrow/pull/36932#discussion_r1288111862
##########
cpp/src/arrow/compute/kernels/vector_cumulative_ops.cc:
##########
@@ -217,53 +253,142 @@ const FunctionDoc cumulative_min_doc{
"start as the new minimum)."),
{"values"},
"CumulativeOptions"};
+
+const FunctionDoc cumulative_mean_doc{
+ "Compute the cumulative mean over a numeric input",
+ ("`values` must be numeric. Return an array/chunked array which is the\n"
+ "cumulative mean computed over `values`. CumulativeOptions::start_value
is \n"
+ "ignored."),
+ {"values"},
+ "CumulativeOptions"};
} // namespace
Review Comment:
The anonymous namespace could encompass more symbols, for example below.
##########
cpp/src/arrow/compute/api_vector.h:
##########
@@ -661,6 +661,16 @@ Result<Datum> CumulativeMin(
const Datum& values, const CumulativeOptions& options =
CumulativeOptions::Defaults(),
ExecContext* ctx = NULLPTR);
+/// \brief Compute the cumulative mean of an array-like object
+///
Review Comment:
Can you note that the start_value option is ignored?
##########
cpp/src/arrow/compute/kernels/vector_cumulative_ops.cc:
##########
@@ -217,53 +253,142 @@ const FunctionDoc cumulative_min_doc{
"start as the new minimum)."),
{"values"},
"CumulativeOptions"};
+
+const FunctionDoc cumulative_mean_doc{
+ "Compute the cumulative mean over a numeric input",
+ ("`values` must be numeric. Return an array/chunked array which is the\n"
+ "cumulative mean computed over `values`. CumulativeOptions::start_value
is \n"
+ "ignored."),
+ {"values"},
+ "CumulativeOptions"};
} // namespace
+// Kernel factory for simple arithmetic operations.
+// Op is a compute kernel representing any binary associative operation with
+// an identity element (add, product, min, max, etc.), i.e. ones that form a
monoid.
+template <typename Op, typename OptionsType>
+struct CumulativeBinaryOpKernelFactory {
Review Comment:
Would something like this work instead:
```c++
// A cumulative kernel factory that forwards to CumulativeBinaryOp<Op, ...>
for the given type
template <typename Op, typename OptionsType,
template <typename ArgType> State = CumulativeBinaryOp<Op, ArgType>>
using CumulativeBinaryOpKernelFactory =
CumulativeStatefulKernelFactory<State, OptionsType>;
```
##########
cpp/src/arrow/compute/kernels/vector_cumulative_ops_test.cc:
##########
@@ -37,19 +37,23 @@
namespace arrow {
namespace compute {
-constexpr static std::array<const char*, 6> kCumulativeFunctionNames{
+constexpr static std::array<const char*, 7> kCumulativeFunctionNames{
Review Comment:
Since we are changing this, can we simply make this a
`std::vector<std::string>`? It will make further maintenance easier.
##########
cpp/src/arrow/compute/api_vector.h:
##########
@@ -661,6 +661,16 @@ Result<Datum> CumulativeMin(
const Datum& values, const CumulativeOptions& options =
CumulativeOptions::Defaults(),
ExecContext* ctx = NULLPTR);
+/// \brief Compute the cumulative mean of an array-like object
+///
Review Comment:
Or add it in the `CumulativeOptions` docs instead.
##########
docs/source/cpp/compute.rst:
##########
@@ -1621,21 +1621,23 @@ do not detect overflow. They are alsoavailable in an
overflow-checking variant,
suffixed ``_checked``, which returns an ``Invalid`` :class:`Status` when
overflow is detected.
-+------------------------+-------+-------------+-------------+--------------------------------+-------+
-| Function name | Arity | Input types | Output type | Options class
| Notes |
-+=========================+=======+=============+=============+================================+=======+
-| cumulative_sum | Unary | Numeric | Numeric |
:struct:`CumulativeOptions` | \(1) |
-+-------------------------+-------+-------------+-------------+--------------------------------+-------+
-| cumulative_sum_checked | Unary | Numeric | Numeric |
:struct:`CumulativeOptions` | \(1) |
-+-------------------------+-------+-------------+-------------+--------------------------------+-------+
-| cumulative_prod | Unary | Numeric | Numeric |
:struct:`CumulativeOptions` | \(1) |
-+-------------------------+-------+-------------+-------------+--------------------------------+-------+
-| cumulative_prod_checked | Unary | Numeric | Numeric |
:struct:`CumulativeOptions` | \(1) |
-+-------------------------+-------+-------------+-------------+--------------------------------+-------+
-| cumulative_max | Unary | Numeric | Numeric |
:struct:`CumulativeOptions` | \(1) |
-+-------------------------+-------+-------------+-------------+--------------------------------+-------+
-| cumulative_min | Unary | Numeric | Numeric |
:struct:`CumulativeOptions` | \(1) |
-+-------------------------+-------+-------------+-------------+--------------------------------+-------+
++-------------------------+-------+-------------+-------------+--------------------------------+-----------+
+| Function name | Arity | Input types | Output type | Options class
| Notes |
++=========================+=======+=============+=============+================================+===========+
+| cumulative_sum | Unary | Numeric | Numeric |
:struct:`CumulativeOptions` | \(1) |
++-------------------------+-------+-------------+-------------+--------------------------------+-----------+
+| cumulative_sum_checked | Unary | Numeric | Numeric |
:struct:`CumulativeOptions` | \(1) |
++-------------------------+-------+-------------+-------------+--------------------------------+-----------+
+| cumulative_prod | Unary | Numeric | Numeric |
:struct:`CumulativeOptions` | \(1) |
++-------------------------+-------+-------------+-------------+--------------------------------+-----------+
+| cumulative_prod_checked | Unary | Numeric | Numeric |
:struct:`CumulativeOptions` | \(1) |
++-------------------------+-------+-------------+-------------+--------------------------------+-----------+
+| cumulative_max | Unary | Numeric | Numeric |
:struct:`CumulativeOptions` | \(1) |
++-------------------------+-------+-------------+-------------+--------------------------------+-----------+
+| cumulative_min | Unary | Numeric | Numeric |
:struct:`CumulativeOptions` | \(1) |
++-------------------------+-------+-------------+-------------+--------------------------------+-----------+
+| cumulative_mean | Unary | Numeric | Double |
:struct:`CumulativeOptions` | \(1) \(2) |
Review Comment:
We use "Float64" not "Double" here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
