Promote int32 to int64 when reducing with sum by SimonHeybrock · Pull Request #3965 · scipp/scipp

SimonHeybrock · 2026-09-07T09:06:44Z

The bug

An int32 sum overflows at 2**31 and wraps, silently:

>>> big = 2**31 - 1
>>> sc.sum(sc.array(dims=['x'], values=[big, big, big], dtype='int32')).value
2147483645          # exact answer is 6442450941

There is no exception and no warning, and the result is not merely imprecise but meaningless — a large positive sum can come back negative.

The rule scipp already has

sum_into and zeros_not_bool_like already implement, between them, a consistent rule that int32 was simply left out of:

Accumulate in a type that can hold the sum. Return the input type if it can represent the result; otherwise return the wider type.

input	accumulate in	return	before
float32	float64	float32	already handled, in `sum_into`
bool	int64	int64	already handled, via `ZeroNotBool`
int32	int64	int64	wrapped
int64	int64	int64	best effort, nothing wider exists

float32 keeps returning float32 because only its precision is ever at stake — its range accommodates any sum of float32 values, and sum_into already restores precision by accumulating in double. bool and int32 are the cases where the range is the problem, so the result type has to widen. That is what the existing comment above sum means by bool "cannot contain its sum"; int32 cannot either.

This also matches numpy, which promotes integer reductions to the default integer type while leaving float32 alone.

FillValue::ZeroNotBool is renamed to ZeroForSum, as it no longer singles out bool.

The masked-groupby bug this uncovers

groupby(...).sum() over masked data builds its mask replacement with the output's fill value, then substitutes it into the data before accumulating. With a promoting fill value the replacement gets the accumulator's dtype rather than the data's, and where has no mixed-dtype overload. So this already fails on main, for bool:

>>> sc.groupby(masked_bool_data_array, 'g').sum('x')
DTypeError: 'where' does not support dtypes 'bool', 'int64', 'bool',

Without a fix, promoting int32 would extend that failure to masked int32 data, which works today. The replacement is consumed before accumulation, so it must match the dtype of the data being masked, not that of the output — the same substitution variable::reduce_to_dims already makes on the dense path. Masked groupby(...).sum() on bool data therefore starts working too.

Effect on callers

sum, nansum, bins_sum, bins_nansum, groupby.sum, groupby.nansum and anything built on them (including hist over int32-weighted binned data) now return int64 for int32 input. Code that relied on int32 in / int32 out gets a wider dtype — and was silently wrong whenever the sum exceeded 2**31.

mean is unaffected in dtype: it already normalized to float64.

Testing

C++: sum/nansum of int32 return int64; a sum of three INT32_MAX values is exact; int64/float32/float64 keep their dtype; special_like gains an int32 case mirroring the bool one; masked groupby.sum covered for both int32 and bool.
Python: promotion for bool and int32 across Variable/DataArray/Dataset/DataGroup, preservation for the other dtypes, and an overflow regression test.

Both bugs above were reproduced against released scipp 26.8.0, and the new Python tests were confirmed to fail against it for the expected reason.

An int32 sum overflows at 2**31 and wraps to a negative value with no indication: summing three copies of INT32_MAX returned 2147483645. scipp already handles the same situation for bool, whose sum is a count that bool cannot hold, by accumulating into an int64 variable. int32 is the identical case and was simply missed. The rule the code already implements is now stated in one place: accumulate in a type that can hold the sum, and return the input type if it can represent the result, otherwise the wider type. float32 keeps returning float32 because only its precision is at stake, not its range, and sum_into already accumulates it in double. FillValue::ZeroNotBool is renamed to ZeroForSum, since it no longer singles out bool. Also fixes a pre-existing bug the promotion would otherwise have extended to int32: groupby(...).sum() on masked data built its mask replacement with the output's fill value, so for bool it produced an int64 zero to substitute into bool data and threw DTypeError: 'where' does not support dtypes 'bool', 'int64', 'bool' The replacement is consumed before accumulation, so it must match the data's dtype rather than the accumulator's -- the same substitution reduce_to_dims already makes for the dense path.

`long` is 32 bits on Windows, so the expected value in the overflow test overflowed in the test itself rather than in scipp. `DatasetShapeChangingOpTest.sum_masked` is typed over int32, whose sum is now int64.

GroupbyMaskedDataArrayTest.sum sums data declared as `int`, which is int32 and so now yields int64. It was missed because the search for affected tests looked for `int32_t` and this file spells the same type `int`. The new masked_bool test compared against a dimensionless variable, but a bool variable defaults to unit `none` and the sum preserves the input's unit.

A bool array has unit None, which the sum preserves, while sc.scalar defaults to dimensionless. Unrelated to the promotion itself: bool already promoted before this branch.

jl-wynen

I am wondering, do we need the ability to detect and signal these kinds of problems? NumPy raises warnings on overflow as well as division by zero. Should we look into implementing this as well? It would catch overflow in int64. (This would be on top of this PR, not instead.)

jl-wynen · 2026-09-08T08:42:12Z

 TYPED_TEST(DatasetShapeChangingOpTest, sum_masked) {
+  // int32 cannot contain its own sum, so the result is int64.
+  using Result = std::conditional_t<std::is_same_v<TypeParam, int32_t>, int64_t,
+                                    TypeParam>;


Is there a test for bool?

Good catch -- there was none for dense data. Added sum_masked_bool as a separate test: bool cannot join DataTypes, since the fixture's values make the mean expectations of the other tests in that suite meaningless.

jl-wynen · 2026-09-08T08:46:42Z

    would be obtained from manually summing individual dimensions.

+    Inputs of dtype 'bool' or 'int32' cannot contain their own sum, so the result
+    has dtype 'int64'. Other dtypes are preserved.


This is a bit too simplistic. int32 can contain many sums. And int64 can also overflow.

jl-wynen · 2026-09-08T08:48:45Z

+  EXPECT_EQ(sum(var, Dim::X), makeVariable<int64_t>(Dims{Dim::Y}, Shape{2},
+                                                    sc_units::m, Values{4, 6}));
+  EXPECT_EQ(nansum(var), makeVariable<int64_t>(sc_units::m, Values{10}));
+}


Is this not covered by parametrized tests above?

Mostly yes: NansumTest covers int32 for both all-dims and with-dim, and the dtype claim is covered more sharply by the two tests right below. Removed.

jl-wynen · 2026-09-08T09:00:25Z

+    special_like, [](const auto &x) {
+      using T = std::decay_t<decltype(x)>;


Just a nitpick, but we use C++ 23 now:

Suggested change

special_like, [](const auto &x) {

using T = std::decay_t<decltype(x)>;

special_like, []<class T>(const T &x) {

jl-wynen · 2026-09-08T10:44:27Z

  const auto mask_replacement =
-      special_like(Variable(data.data(), Dimensions{}), fill);
+      special_like(Variable(data.data(), Dimensions{}),
+                   fill == FillValue::ZeroForSum ? FillValue::Default : fill);


Why do you handle ZeroForSum here instead of in special_like?

Producing the promoted dtype is exactly what special_like does for ZeroForSum -- it builds the accumulator. The mask replacement is the opposite case: it is substituted into the input before accumulation, so it must keep the input's dtype.

You are right that it should not be spelled out twice, though -- the same ternary and comment sat in reduce_to_dims. Both now call mask_fill_value in core/flags.h, which carries the explanation (including a note on the alternative of dropping ZeroForSum and making the promotion a property of the reduction).

- Extract the ZeroForSum -> Default substitution for masked input elements into `mask_fill_value`, replacing the duplicated ternary and comment in `reduce_to_dims` and `groupby::reduce_`. - Reword the claim that bool and int32 "cannot contain their own sum": they are promoted because sums leave their range quickly, and int64 can still overflow silently. - Add a masked bool sum test for Dataset. bool cannot join the typed fixture, whose `mean` expectations are not meaningful for it. - Drop `sum_int32_accumulates_in_int64`, covered by the parametrized nansum tests and the two dtype tests next to it. - Use a C++23 templated lambda in `zeros_for_sum_like`.

The C++20 abbreviated template lambda introduced in 3b924cd broke CI on two of three platforms: MSVC fails to compile zero_init<T>::value() for Eigen::Vector3d (C2440), and on Ubuntu VectorReduceTest.sum_vector sums to the wrong result. Bisecting the branch's CI runs pins this to that lambda specifically -- it's the only change in that commit touching the Eigen::Vector3d dispatch path. The plain auto/decay_t lambda is functionally identical and is what passed on all platforms before. Co-Authored-By: Claude Sonnet 5 <[email protected]>

SimonHeybrock mentioned this pull request Sep 7, 2026

Accumulate detector view histograms in int32 once scipp promotes integer reductions scipp/esslivedata#1293

Closed

SimonHeybrock added 3 commits September 7, 2026 09:39

Fix tests for the int32 sum promotion

6708c56

`long` is 32 bits on Windows, so the expected value in the overflow test overflowed in the test itself rather than in scipp. `DatasetShapeChangingOpTest.sum_masked` is typed over int32, whose sum is now int64.

Fix unit in the bool half of the sum promotion test

b799d52

A bool array has unit None, which the sum preserves, while sc.scalar defaults to dimensionless. Unrelated to the promotion itself: bool already promoted before this branch.

jl-wynen mentioned this pull request Sep 8, 2026

Add saturating arithmetic #3967

Open

jl-wynen reviewed Sep 8, 2026

View reviewed changes

jl-wynen mentioned this pull request Sep 9, 2026

Raise warnings when operations go wrong but don't fail completely #3971

Open

jl-wynen approved these changes Sep 10, 2026

View reviewed changes

SimonHeybrock enabled auto-merge September 11, 2026 06:43

Merge branch 'main' into int32-sum-promotion

2624d3a

SimonHeybrock merged commit 158230d into main Sep 11, 2026
4 checks passed

SimonHeybrock deleted the int32-sum-promotion branch September 11, 2026 07:14

SimonHeybrock mentioned this pull request Sep 22, 2026

Accumulate detector view histograms in int32 scipp/esslivedata#1323

Merged

1 of 2 tasks

Navigation Menu

Repository navigation

Promote int32 to int64 when reducing with sum - #3965

Promote int32 to int64 when reducing with sum#3965
SimonHeybrock merged 7 commits into
mainfrom
int32-sum-promotion

SimonHeybrock commented Sep 7, 2026 •

edited

Loading

Uh oh!

jl-wynen left a comment

Uh oh!

jl-wynen Sep 8, 2026

Uh oh!

SimonHeybrock Sep 8, 2026

Uh oh!

jl-wynen Sep 8, 2026

Uh oh!

jl-wynen Sep 8, 2026

Uh oh!

SimonHeybrock Sep 8, 2026

Uh oh!

jl-wynen Sep 8, 2026

Uh oh!

jl-wynen Sep 8, 2026

Uh oh!

SimonHeybrock Sep 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		special_like, [](const auto &x) {
		using T = std::decay_t<decltype(x)>;

	special_like, [](const auto &x) {
	using T = std::decay_t<decltype(x)>;
	special_like, []<class T>(const T &x) {

Conversation

SimonHeybrock commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The bug

The rule scipp already has

The masked-groupby bug this uncovers

Effect on callers

Testing

Uh oh!

jl-wynen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SimonHeybrock commented Sep 7, 2026 •

edited

Loading