From e5bf7c51cb579ff203b3963eb9e7ff7ca444aa82 Mon Sep 17 00:00:00 2001 From: Lars Bogner Date: Thu, 2 Jul 2026 16:54:02 +0200 Subject: [PATCH] Fix silent failure modes surfaced by extensive code review - energy_simplex_encode: warn when clipping post_E to pre_E discards recorded edep/e_sec instead of silently zeroing them - local/inv_local_frame_rotation: validate and normalize pre_dir instead of silently assuming unit norm; raise on near-zero-norm rows - train(): make --lr authoritative on resume instead of being silently overwritten by the checkpoint's optimizer/scheduler state; print and exit cleanly instead of silently training zero epochs when the checkpoint already meets --epochs; truncate metrics.csv on a fresh run instead of always appending - dwarf update-manifest: check file existence for every manifest line, not just ones whose gen/schema actually changed - pyproject.toml: dev extra now pulls in convert+analysis so the documented `uv sync --extra cpu --extra dev` + `pytest` actually passes collection Co-Authored-By: Claude Sonnet 5 --- giant/data/transforms.py | 44 +++++++++++++++++++++++++++++++-- giant/train.py | 23 +++++++++++++++-- pyproject.toml | 1 + scripts/bump_dataset_version.py | 11 ++++++--- tests/test_transforms.py | 31 +++++++++++++++++++++++ uv.lock | 6 +++++ 6 files changed, 108 insertions(+), 8 deletions(-) diff --git a/giant/data/transforms.py b/giant/data/transforms.py index 0cb8432..ac1e8ab 100644 --- a/giant/data/transforms.py +++ b/giant/data/transforms.py @@ -1,3 +1,5 @@ +import warnings + import numpy as np _EPS = 1e-8 @@ -37,10 +39,26 @@ def energy_simplex_encode( `energy_simplex_decode` is its inverse (up to the floor softening). """ pre_E = np.maximum(np.asarray(pre_E, dtype=np.float32), _EPS) - post_E = np.clip(np.asarray(post_E, dtype=np.float32), 0.0, pre_E) + raw_post_E = np.asarray(post_E, dtype=np.float32) + post_E = np.clip(raw_post_E, 0.0, pre_E) delta_e = pre_E - post_E - lost = np.asarray(edep, dtype=np.float32) + np.asarray(e_sec, dtype=np.float32) + edep = np.asarray(edep, dtype=np.float32) + e_sec = np.asarray(e_sec, dtype=np.float32) + lost = edep + e_sec has_loss = lost > _EPS + # Clipping post_E down to pre_E forces delta_e (and thus the rescaled + # edep/e_sec below) to 0 even on rows where edep/e_sec were genuinely + # recorded as nonzero — warn so this doesn't silently discard real data. + discarded = (raw_post_E > pre_E) & has_loss + if np.any(discarded): + n = int(np.sum(discarded)) + warnings.warn( + f"energy_simplex_encode: {n}/{len(discarded)} step(s) had " + "post_E > pre_E (clipped) while recording nonzero edep/e_sec; " + "that recorded energy deposit is discarded to keep delta_e " + "consistent with the clip.", + stacklevel=2, + ) scale = np.where(has_loss, delta_e / np.maximum(lost, _EPS), 0.0) # Where nothing was recorded as deposited/secondary but energy was lost, # attribute all of delta_e to local deposit. @@ -108,6 +126,26 @@ def _cross_with_z_axis(axis: np.ndarray, v: np.ndarray) -> np.ndarray: return np.concatenate([ay * bz, -ax * bz, ax * by - ay * bx], axis=1) +def _validate_unit_pre_dir(pre_dir: np.ndarray) -> np.ndarray: + """Normalize pre_dir and raise if any row is too degenerate to define a frame. + + `local_frame_rotation`/`inv_local_frame_rotation` treat pre_dir[:, 2] as + cos(angle to ẑ), which is only correct for a unit vector. Small float32 + drift is corrected silently; a near-zero-norm row has no well-defined + direction, so it's raised loudly instead of producing a meaningless + rotation (previously it fell through to an arbitrary axis with no error). + """ + pre_dir = np.asarray(pre_dir, dtype=np.float32) + norm = np.linalg.norm(pre_dir, axis=1, keepdims=True) + if np.any(norm < 1e-6): + raise ValueError( + f"pre_dir has {int(np.sum(norm < 1e-6))} row(s) with near-zero norm " + "(< 1e-6); local/inv_local_frame_rotation require a well-defined " + "incoming direction for every row." + ) + return pre_dir / norm + + def local_frame_rotation(pre_dir: np.ndarray, post_dir: np.ndarray) -> np.ndarray: """Rotate post_dir into the local frame where pre_dir maps to ẑ (Rodrigues). @@ -115,6 +153,7 @@ def local_frame_rotation(pre_dir: np.ndarray, post_dir: np.ndarray) -> np.ndarra expressed relative to a coordinate system in which the incoming particle travels along +z. """ + pre_dir = _validate_unit_pre_dir(pre_dir) cos_t = np.clip(pre_dir[:, 2:3], -1.0, 1.0) # (N,1); dot with ẑ = z-component sin_t = np.sqrt(np.maximum(0.0, 1.0 - cos_t**2)) # (N,1) @@ -227,6 +266,7 @@ def inv_local_frame_rotation( Applies R^T (same axis, negative angle) to post_dir_local. """ + pre_dir = _validate_unit_pre_dir(pre_dir) cos_t = np.clip(pre_dir[:, 2:3], -1.0, 1.0) # dot with ẑ = z-component sin_t = np.sqrt(np.maximum(0.0, 1.0 - cos_t**2)) diff --git a/giant/train.py b/giant/train.py index 894c47b..a95333e 100644 --- a/giant/train.py +++ b/giant/train.py @@ -103,9 +103,28 @@ def train( start_epoch = ckpt.get("epoch", 0) + 1 best_val_loss = ckpt.get("best_val_loss", float("inf")) + # optimizer/lr_sched.load_state_dict() above restore the checkpoint's + # own base LR, which would otherwise silently override an explicit + # `lr` argument. Make `lr` authoritative again, applied at whatever + # point the cosine/warmup schedule has already reached. + lr_sched.base_lrs = [lr for _ in lr_sched.base_lrs] + resumed_lr = lr * _lr_lambda(lr_sched.last_epoch) + for group in optimizer.param_groups: + group["lr"] = resumed_lr + + if start_epoch > epochs: + print( + f"checkpoint already completed epoch {start_epoch - 1} " + f"(>= --epochs {epochs}) — nothing to train" + ) + return + metrics_path = out_dir / "metrics.csv" - write_header = not (resume_path is not None and metrics_path.exists()) - metrics_file = open(metrics_path, "a", newline="") + resuming_existing_metrics = resume_path is not None and metrics_path.exists() + write_header = not resuming_existing_metrics + metrics_file = open( + metrics_path, "a" if resuming_existing_metrics else "w", newline="" + ) metrics_writer = csv.DictWriter(metrics_file, fieldnames=_METRICS_FIELDS) if write_header: metrics_writer.writeheader() diff --git a/pyproject.toml b/pyproject.toml index 997b936..582f397 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -24,6 +24,7 @@ dev = [ "pytest>=8,<10", "ruff>=0.15,<1", "ty>=0.0.50,<0.1", + "giant[convert,analysis]", ] convert = [ "uproot>=5.3,<6", diff --git a/scripts/bump_dataset_version.py b/scripts/bump_dataset_version.py index 633ce4b..d371441 100644 --- a/scripts/bump_dataset_version.py +++ b/scripts/bump_dataset_version.py @@ -472,14 +472,17 @@ def plan_update_manifest( parts[schema_idx] = new_schema changed = True - if not changed: - result.append((raw, None)) - continue - + # Check existence for every data line, not just ones whose gen/schema + # actually changed — an already-correct-looking line can still point + # at a file that was deleted or moved out-of-band. new_abs = Path(*parts) if not new_abs.exists(): missing.append(new_abs) + if not changed: + result.append((raw, None)) + continue + new_rel = os.path.relpath(new_abs, start=manifest_dir) result.append((raw, new_rel)) diff --git a/tests/test_transforms.py b/tests/test_transforms.py index fef8fa5..b202cb0 100644 --- a/tests/test_transforms.py +++ b/tests/test_transforms.py @@ -1,7 +1,9 @@ import numpy as np +import pytest from giant.data.transforms import ( energy_simplex_decode, energy_simplex_encode, + inv_local_frame_rotation, inv_log_transform, local_frame_rotation, log_transform, @@ -53,6 +55,35 @@ def test_local_frame_rotation_preserves_norm(): np.testing.assert_allclose(np.linalg.norm(result, axis=1), 1.0, atol=1e-5) +def test_local_frame_rotation_rejects_near_zero_pre_dir(): + """A degenerate (near-zero-norm) pre_dir has no well-defined frame — must + raise instead of silently falling back to an arbitrary rotation axis.""" + pre_dir = np.array([[0.0, 0.0, 0.0], [0.0, 0.0, 1.0]], dtype=np.float32) + post_dir = np.array([[1.0, 0.0, 0.0], [0.0, 1.0, 0.0]], dtype=np.float32) + with pytest.raises(ValueError, match="near-zero norm"): + local_frame_rotation(pre_dir, post_dir) + with pytest.raises(ValueError, match="near-zero norm"): + inv_local_frame_rotation(pre_dir, post_dir) + + +def test_local_frame_rotation_normalizes_non_unit_pre_dir(): + """A pre_dir with float32-drift norm (not exactly 1) must still produce the + same result as its exactly-normalized counterpart, not a skewed frame.""" + rng = np.random.default_rng(9) + N = 50 + pre_dir_unit = rng.standard_normal((N, 3)).astype(np.float32) + pre_dir_unit /= np.linalg.norm(pre_dir_unit, axis=1, keepdims=True) + post_dir = rng.standard_normal((N, 3)).astype(np.float32) + post_dir /= np.linalg.norm(post_dir, axis=1, keepdims=True) + + pre_dir_scaled = pre_dir_unit * rng.uniform(0.9, 1.1, size=(N, 1)).astype( + np.float32 + ) + expected = local_frame_rotation(pre_dir_unit, post_dir) + result = local_frame_rotation(pre_dir_scaled, post_dir) + np.testing.assert_allclose(result, expected, atol=1e-4) + + def test_travel_direction_is_unit_norm(): rng = np.random.default_rng(5) N = 50 diff --git a/uv.lock b/uv.lock index 81134bb..aba0c58 100644 --- a/uv.lock +++ b/uv.lock @@ -464,14 +464,20 @@ cuda = [ { name = "torch", version = "2.3.1+cu118", source = { registry = "https://download.pytorch.org/whl/cu118" } }, ] dev = [ + { name = "awkward" }, + { name = "ipykernel" }, + { name = "matplotlib" }, + { name = "polars" }, { name = "pytest" }, { name = "ruff" }, { name = "ty" }, + { name = "uproot" }, ] [package.metadata] requires-dist = [ { name = "awkward", marker = "extra == 'convert'", specifier = ">=2.6,<3" }, + { name = "giant", extras = ["convert", "analysis"], marker = "extra == 'dev'" }, { name = "ipykernel", marker = "extra == 'analysis'", specifier = ">=7.3.0" }, { name = "matplotlib", marker = "extra == 'analysis'", specifier = ">=3.8,<4" }, { name = "numpy", specifier = ">=1.26,<3" },