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 <noreply@anthropic.com>
This commit is contained in:
@@ -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))
|
||||
|
||||
|
||||
+21
-2
@@ -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()
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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))
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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" },
|
||||
|
||||
Reference in New Issue
Block a user