Skip to content

Commit f95b001

Browse files
Adarsh-MeAdarsh-Me
authored
GH-51194: [C++] Fix cumulative_max/min default start for floating-point types (#51203)
## Rationale for this change `Identity<Max>` (the implicit default start of `cumulative_max`) used `std::numeric_limits<T>::min()`, which is the smallest **positive** value for floating-point types. Any input starting with a non-positive value therefore never replaced the start as the new maximum: ```python >>> import pyarrow as pa, pyarrow.compute as pc >>> pc.cumulative_max(pa.array([-2.5, 2.5])).to_pylist() [2.2250738585072014e-308, 2.5] # expected [-2.5, 2.5] ``` ## What changes are included in this PR? - `Identity<Max>` now uses `-infinity` for floating-point types (and half-float), and keeps `lowest()` for integer types. `-infinity` is the only value satisfying the documented identity property `Op(identity, x) = x for all x` — `lowest()` (most negative finite value) fails it for a leading `-inf` input. - `Identity<Min>` gets the mirror fix: `+infinity` for floating-point types. Previously `cumulative_min([inf, 1.0])` returned `[DBL_MAX, 1.0]` instead of `[inf, 1.0]`. Closes #51194. ## Are these changes tested? - Added `TestCumulative.NegativeValues` covering negative integers, negative floats, and `±Inf` inputs for both `cumulative_max` and `cumulative_min`. - Added a pyarrow regression test reproducing the issue (`test_cumulative_max_min_negative_default_start`). ## Are there any user-facing changes? Yes: `cumulative_max`/`cumulative_min` now return correct results for floating-point inputs whose first value is non-positive (for max) / non-negative or `inf` (for min). The documented behavior — 'the default start is the minimum/maximum value of input type' — is now actually honored. * GitHub Issue: #51194 Authored-by: Adarsh <adarsh@neuroad.ai> Signed-off-by: Antoine Pitrou <antoine@python.org>
1 parent e235bdd commit f95b001

3 files changed

Lines changed: 65 additions & 2 deletions

File tree

‎cpp/src/arrow/compute/kernels/base_arithmetic_internal.h‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -759,15 +759,27 @@ template <>
759759
struct Identity<Max> {
760760
template <typename Value>
761761
static constexpr Value value() {
762-
return std::numeric_limits<Value>::min();
762+
// Note that `min()` returns the smallest positive value for
763+
// floating-point types, and `lowest()` doesn't satisfy the identity
764+
// property for -inf inputs, so use -infinity for those types.
765+
if constexpr (std::is_floating_point_v<Value> || std::is_same_v<Float16, Value>) {
766+
return -std::numeric_limits<Value>::infinity();
767+
} else {
768+
return std::numeric_limits<Value>::lowest();
769+
}
763770
}
764771
};
765772

766773
template <>
767774
struct Identity<Min> {
768775
template <typename Value>
769776
static constexpr Value value() {
770-
return std::numeric_limits<Value>::max();
777+
// Mirror of Identity<Max>: use +infinity for floating-point types.
778+
if constexpr (std::is_floating_point_v<Value> || std::is_same_v<Float16, Value>) {
779+
return std::numeric_limits<Value>::infinity();
780+
} else {
781+
return std::numeric_limits<Value>::max();
782+
}
771783
}
772784
};
773785

‎cpp/src/arrow/compute/kernels/vector_cumulative_ops_test.cc‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -949,5 +949,34 @@ TEST(TestCumulative, NaN) {
949949
CheckVectorUnary("cumulative_mean", ArrayFromJSON(float64(), "[5, 4, NaN, 2, 1]"),
950950
ArrayFromJSON(float64(), "[5, 4.5, NaN, NaN, NaN]"));
951951
}
952+
953+
TEST(TestCumulative, NegativeValues) {
954+
// GH-51194: the default start for cumulative_max was initialized with
955+
// std::numeric_limits<T>::min(), which is the smallest positive value for
956+
// floating-point types, so non-positive values never replaced the start
957+
CumulativeOptions options;
958+
for (auto ty : SignedIntTypes()) {
959+
CheckVectorUnary("cumulative_max", ArrayFromJSON(ty, "[-2, -1, -3]"),
960+
ArrayFromJSON(ty, "[-2, -1, -1]"), &options);
961+
CheckVectorUnary("cumulative_min", ArrayFromJSON(ty, "[-2, -1, -3]"),
962+
ArrayFromJSON(ty, "[-2, -2, -3]"), &options);
963+
}
964+
965+
for (auto ty : FloatingPointTypes()) {
966+
CheckVectorUnary("cumulative_max", ArrayFromJSON(ty, "[-2.5, 2.5]"),
967+
ArrayFromJSON(ty, "[-2.5, 2.5]"), &options);
968+
CheckVectorUnary("cumulative_min", ArrayFromJSON(ty, "[-2.5, 2.5]"),
969+
ArrayFromJSON(ty, "[-2.5, -2.5]"), &options);
970+
CheckVectorUnary("cumulative_max", ArrayFromJSON(ty, "[-2.5, -1.5, -3.5, -0.5]"),
971+
ArrayFromJSON(ty, "[-2.5, -1.5, -1.5, -0.5]"), &options);
972+
973+
// The default start must compare lower (higher for min) than every value
974+
// of the type, including infinities
975+
CheckVectorUnary("cumulative_max", ArrayFromJSON(ty, "[-Inf, -2.5]"),
976+
ArrayFromJSON(ty, "[-Inf, -2.5]"), &options);
977+
CheckVectorUnary("cumulative_min", ArrayFromJSON(ty, "[Inf, 2.5]"),
978+
ArrayFromJSON(ty, "[Inf, 2.5]"), &options);
979+
}
980+
}
952981
} // namespace compute
953982
} // namespace arrow

‎python/pyarrow/tests/test_compute.py‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3625,6 +3625,28 @@ def test_cumulative_max(start, skip_nulls):
36253625
pc.cumulative_max([1, 2, 3], start=strt)
36263626

36273627

3628+
@pytest.mark.numpy
3629+
def test_cumulative_max_min_negative_default_start():
3630+
# GH-51194: the implicit start for cumulative_max was initialized with
3631+
# std::numeric_limits<T>::min(), which is the smallest positive value for
3632+
# floating-point types, so non-positive values never replaced the start
3633+
values = [-2.5, 2.5]
3634+
arr = pa.array(values, type=pa.float64())
3635+
assert pc.cumulative_max(arr).to_pylist() == [-2.5, 2.5]
3636+
assert pc.cumulative_min(arr).to_pylist() == [-2.5, -2.5]
3637+
3638+
arr = pa.chunked_array([[-2.5, -1.5], [-3.5, -0.5]])
3639+
assert pc.cumulative_max(arr).to_pylist() == [-2.5, -1.5, -1.5, -0.5]
3640+
assert pc.cumulative_min(arr).to_pylist() == [-2.5, -2.5, -3.5, -3.5]
3641+
3642+
# The default start must compare lower (higher for min) than every value
3643+
# of the type, including infinities
3644+
arr = pa.array([-np.inf, -2.5], type=pa.float64())
3645+
assert pc.cumulative_max(arr).to_pylist() == [-np.inf, -2.5]
3646+
arr = pa.array([np.inf, 2.5], type=pa.float64())
3647+
assert pc.cumulative_min(arr).to_pylist() == [np.inf, 2.5]
3648+
3649+
36283650
@pytest.mark.numpy
36293651
@pytest.mark.parametrize('start', (0.5, 3.5, 6.5))
36303652
@pytest.mark.parametrize('skip_nulls', (True, False))

0 commit comments

Comments
 (0)