Analysis histogram bin index clips after the i32 cast, not before (compute-one crashes on extreme rollout values) #61

New Issue

2026-08-17T09:27:52+02:00

lars commented

2026-08-17 09:27:52 +02:00

_bin_expr in giant/analysis/reduce.py clips the bin index to [0, nbins-1] only after casting it to Int32, so the clip never gets a chance to run: a rollout step_length of 1.0725e10 mm against fixed edges [2.9e-5, 94.04] with 50 bins produces a raw index of ~5.7e9, which overflows i32 and fails the strict cast — killing the whole compute-one job. Same failure mode for +/-inf.

Fix: clamp in f64 first, then cast to Int32. NaN has no edge to clamp to, so map it to null and drop it in the two callers (hist1d, profile_partial) — matching what np.histogram does with NaN, and what profile_partial needs anyway since a null bin index would break its np.add.at.

This was already fixed once in commit 313373c ("Clamp analysis histogram bins before the i32 cast, not after"), but that commit landed on a branch (fix/rollout-negative-secondary-mass) that forked off a stale master and was never merged — giant/analysis/reduce.py has since diverged enough that it doesn't apply cleanly. Needs to be reimplemented against current master. The original commit's diff (including its two new regression tests in tests/test_analysis_reduce.py) is a good reference: git show 313373cc10d77d9c452c0b13fd62f8770a963590.

`_bin_expr` in `giant/analysis/reduce.py` clips the bin index to `[0, nbins-1]` only *after* casting it to `Int32`, so the clip never gets a chance to run: a rollout `step_length` of `1.0725e10` mm against fixed edges `[2.9e-5, 94.04]` with 50 bins produces a raw index of ~5.7e9, which overflows i32 and fails the strict cast — killing the whole `compute-one` job. Same failure mode for `+/-inf`. Fix: clamp in f64 first, then cast to Int32. NaN has no edge to clamp to, so map it to null and drop it in the two callers (`hist1d`, `profile_partial`) — matching what `np.histogram` does with NaN, and what `profile_partial` needs anyway since a null bin index would break its `np.add.at`. This was already fixed once in commit 313373c ("Clamp analysis histogram bins before the i32 cast, not after"), but that commit landed on a branch (`fix/rollout-negative-secondary-mass`) that forked off a stale master and was never merged — `giant/analysis/reduce.py` has since diverged enough that it doesn't apply cleanly. Needs to be reimplemented against current `master`. The original commit's diff (including its two new regression tests in `tests/test_analysis_reduce.py`) is a good reference: `git show 313373cc10d77d9c452c0b13fd62f8770a963590`.

lars added the bug label 2026-08-17 09:27:52 +02:00

lars commented

2026-08-17 09:40:16 +02:00

Fixed in a746efb on fix/issue-61.

_bin_expr now clamps in f64 before the Int32 cast, and NaN maps to null instead (dropped in hist1d/profile_partial via drop_nulls) rather than crashing the cast. This reimplements 313373c, whose fix and two regression tests (test_hist1d_clamps_extreme_values_and_drops_nan, test_profile_partial_clamps_extreme_values_and_drops_nan) never made it to master since that commit's branch (fix/rollout-negative-secondary-mass) was never merged and reduce.py has since diverged. Both tests were confirmed to fail on unpatched master (cast-overflow exception) and pass after the fix. Full check suite (pytest, ruff format/check, ty) all clean. No follow-up filed.

Fixed in a746efb on fix/issue-61. _bin_expr now clamps in f64 before the Int32 cast, and NaN maps to null instead (dropped in hist1d/profile_partial via drop_nulls) rather than crashing the cast. This reimplements 313373c, whose fix and two regression tests (test_hist1d_clamps_extreme_values_and_drops_nan, test_profile_partial_clamps_extreme_values_and_drops_nan) never made it to master since that commit's branch (fix/rollout-negative-secondary-mass) was never merged and reduce.py has since diverged. Both tests were confirmed to fail on unpatched master (cast-overflow exception) and pass after the fix. Full check suite (pytest, ruff format/check, ty) all clean. No follow-up filed.

lars closed this issue

2026-08-17 09:40:16 +02:00

lars referenced this issue from a commit

2026-08-17 09:42:44 +02:00

Clamp analysis histogram bins before the i32 cast, not after (gitea #61)

lars referenced this issue

2026-08-17 09:43:26 +02:00

Clamp analysis histogram bins before the i32 cast, not after (gitea #61) #63

lars referenced this issue from a commit

2026-08-17 10:17:35 +02:00

Merge pull request 'Clamp analysis histogram bins before the i32 cast, not after (gitea #61)' (#63) from fix/issue-61 into master

Sign in to join this conversation.