Clamp analysis histogram bins before the i32 cast, not after (gitea #61)
CI / Format (ruff format) (push) Successful in 27s
CI / Lint (ruff check) (push) Successful in 28s
CI / Sync project version with tag (push) Has been skipped
CI / Type check (ty) (push) Successful in 36s
CI / Lint (ruff check) (pull_request) Successful in 41s
CI / Format (ruff format) (pull_request) Successful in 40s
CI / Sync project version with tag (pull_request) Has been skipped
CI / Type check (ty) (pull_request) Successful in 40s
CI / Tests (push) Successful in 3m45s
CI / Tests (pull_request) Successful in 1m57s
CI / Format (ruff format) (push) Successful in 27s
CI / Lint (ruff check) (push) Successful in 28s
CI / Sync project version with tag (push) Has been skipped
CI / Type check (ty) (push) Successful in 36s
CI / Lint (ruff check) (pull_request) Successful in 41s
CI / Format (ruff format) (pull_request) Successful in 40s
CI / Sync project version with tag (pull_request) Has been skipped
CI / Type check (ty) (pull_request) Successful in 40s
CI / Tests (push) Successful in 3m45s
CI / Tests (pull_request) Successful in 1m57s
_bin_expr in giant/analysis/reduce.py clipped the bin index to
[0, nbins-1] only after casting it to Int32, so the clip never got 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.
Clamp in f64 first, then cast to Int32. NaN has no edge to clamp to,
so it maps to null and is dropped 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 reimplements commit 313373c, which fixed the same bug but landed
on a branch (fix/rollout-negative-secondary-mass) that forked off a
stale master and was never merged; reduce.py has since diverged enough
that the original diff no longer applies cleanly.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -29,8 +29,17 @@ from giant.constants import TERM_ESCAPED
|
||||
|
||||
|
||||
def _bin_expr(value: pl.Expr, lo: float, hi: float, nbins: int) -> pl.Expr:
|
||||
"""Uniform bin index of ``value`` over ``[lo, hi]`` into ``nbins`` bins."""
|
||||
return ((value - lo) / (hi - lo) * nbins).floor().cast(pl.Int32).clip(0, nbins - 1)
|
||||
"""Uniform bin index of ``value`` over ``[lo, hi]`` into ``nbins`` bins.
|
||||
|
||||
Out-of-range values clamp into the edge bins, and the clamp deliberately
|
||||
happens in f64 *before* the integer cast: a rollout is free to emit a wildly
|
||||
out-of-range outlier (a step_length of 1e10 mm, say) or an inf, whose
|
||||
unclamped bin index overflows i32 and makes the cast fail outright. NaN has
|
||||
no edge to clamp to, so it becomes null and is dropped by the callers below
|
||||
— the same thing ``np.histogram`` does with it.
|
||||
"""
|
||||
idx = ((value - lo) / (hi - lo) * nbins).floor().clip(0, nbins - 1)
|
||||
return pl.when(idx.is_nan()).then(None).otherwise(idx).cast(pl.Int32)
|
||||
|
||||
|
||||
def hist1d(
|
||||
@@ -50,6 +59,7 @@ def hist1d(
|
||||
group = pl.lit(0, dtype=pl.Int64) if group is None else group
|
||||
res = (
|
||||
lf.select(group.alias("_g"), _bin_expr(value, lo, hi, nbins).alias("_b"))
|
||||
.drop_nulls("_b")
|
||||
.group_by("_g", "_b")
|
||||
.agg(pl.len().alias("_n"))
|
||||
.collect(engine="streaming")
|
||||
@@ -188,6 +198,7 @@ def profile_partial(
|
||||
_bin_expr(coord, lo, hi, nbins).alias("_b"),
|
||||
weight.alias("_w"),
|
||||
)
|
||||
.drop_nulls("_b")
|
||||
.group_by("event_id", "_b")
|
||||
.agg(pl.col("_w").sum().alias("_ws"))
|
||||
.collect(engine="streaming")
|
||||
|
||||
@@ -102,6 +102,31 @@ def test_hist1d_overall_and_grouped():
|
||||
assert hg[11].sum() == 4
|
||||
|
||||
|
||||
def test_hist1d_clamps_extreme_values_and_drops_nan():
|
||||
# A rollout can emit a wildly out-of-range step_length (or an inf/NaN); the
|
||||
# fixed-edge binning must clamp rather than overflow the i32 bin cast.
|
||||
lf = pl.DataFrame({"x": [5.0, 1.0725e10, float("inf"), -float("inf"), float("nan"), None]}).lazy()
|
||||
edges = np.linspace(0.0, 50.0, 6) # width 10
|
||||
h = R.hist1d(lf, pl.col("x"), edges)
|
||||
# 5 -> bin 0; 1e10 and +inf -> top bin; -inf -> bin 0; NaN/null dropped
|
||||
assert h[0].tolist() == [2, 0, 0, 0, 2]
|
||||
|
||||
|
||||
def test_profile_partial_clamps_extreme_values_and_drops_nan():
|
||||
lf = pl.DataFrame(
|
||||
{
|
||||
"event_id": [1, 1, 1, 1],
|
||||
"z": [5.0, 1.0725e10, float("nan"), 45.0],
|
||||
"w": [1.0, 2.0, 4.0, 8.0],
|
||||
}
|
||||
).lazy()
|
||||
edges = np.linspace(0.0, 50.0, 6)
|
||||
ev, mat = R.profile_partial(lf, pl.col("z"), edges, pl.col("w"))
|
||||
assert ev.tolist() == [1]
|
||||
# 1e10 clamps into the top bin alongside 45; the NaN row's weight is dropped
|
||||
assert mat[0].tolist() == [1.0, 0.0, 0.0, 0.0, 10.0]
|
||||
|
||||
|
||||
def test_physical_steps_drops_synthetic_rollout_rows_only():
|
||||
lf = _rollout_frame()
|
||||
phys = physical_steps(lf, Side.rollout).collect()
|
||||
|
||||
Reference in New Issue
Block a user