Add data-integrity guards against silent NaN/Inf propagation and races
- log_transform / _validate_unit_pre_dir now raise on non-finite input instead of letting a NaN row silently poison the persisted normalizer cache (norm < 1e-6 was always False for NaN, so the existing guard never caught it). - encode_secondaries warns when a row's secondary energies cumulatively exceed e_sec, instead of silently saturating the overflowing slot's stick-breaking logit via the _EPS floor. - EVENT_ID_FILE_STRIDE overflow now raises instead of silently colliding two files' event ids together (reintroducing train/val leakage). - make_event_split(val_fraction=0.0) now actually holds out nothing, instead of always forcing at least 1 validation event. - setup_cache.save() is now serialized with a flock, since two concurrent writers (a real scenario on this repo's shared portal/condor machines) could otherwise race and silently drop one writer's freshly-computed cache section. - Documented (no behavior change) the pre_dir ≈ -ẑ antipodal rotation singularity in _rodrigues_axis, which is real but inherent to any single-valued local-frame convention. Each fix has a regression test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -30,6 +30,16 @@ def test_make_event_split_no_empty_sets():
|
||||
assert len(val_set) > 0
|
||||
|
||||
|
||||
def test_make_event_split_val_fraction_zero_holds_out_nothing():
|
||||
"""val_fraction=0.0 is an explicit "train on everything" request and
|
||||
must not be silently overridden into holding out 1 event."""
|
||||
rng = np.random.default_rng(3)
|
||||
event_ids = rng.integers(0, 50, size=1000)
|
||||
train_set, val_set = make_event_split(event_ids, val_fraction=0.0)
|
||||
assert val_set == set()
|
||||
assert train_set == set(np.unique(event_ids).tolist())
|
||||
|
||||
|
||||
def test_make_event_split_reproducible():
|
||||
event_ids = np.arange(100)
|
||||
a_tr, a_val = make_event_split(event_ids, val_fraction=0.1, seed=42)
|
||||
|
||||
@@ -344,6 +344,15 @@ def test_load_event_ids_applies_offset(tmp_path):
|
||||
)
|
||||
|
||||
|
||||
def test_load_event_ids_raises_when_event_id_reaches_stride(tmp_path):
|
||||
"""A raw event_id >= EVENT_ID_FILE_STRIDE would collide into the next
|
||||
file's offset block if silently allowed through — must raise instead."""
|
||||
path = tmp_path / "a.parquet"
|
||||
pd.DataFrame({"event_id": [0, 1, EVENT_ID_FILE_STRIDE]}).to_parquet(path)
|
||||
with pytest.raises(ValueError, match="EVENT_ID_FILE_STRIDE"):
|
||||
load_event_ids(path)
|
||||
|
||||
|
||||
def test_load_steps_applies_offset_to_event_id(tmp_path):
|
||||
path = tmp_path / "a.parquet"
|
||||
_steps_df([0, 1]).to_parquet(path)
|
||||
|
||||
@@ -214,6 +214,41 @@ def test_save_merges_non_colliding_normalizer_keys(tmp_path):
|
||||
assert loaded.normalizers["k2"].n_train_steps == 2
|
||||
|
||||
|
||||
def test_save_is_serialized_against_concurrent_writers(tmp_path):
|
||||
"""Without the flock in setup_cache.save(), two concurrent writers can
|
||||
both load() the same base state and merge their own section in
|
||||
independently, so whichever os.replace() lands last silently drops the
|
||||
other's key — a lost-update race, not a corrupt file. Each of these
|
||||
threads writes a distinct normalizer key many times over; if the
|
||||
load-merge-write critical section isn't actually serialized, at least
|
||||
one thread's key is likely to go missing from the final merged cache."""
|
||||
import threading
|
||||
|
||||
data = _touch_parquet(tmp_path / "shard.parquet")
|
||||
files = [data]
|
||||
setup_cache.save(data, files, SetupCache.empty(files))
|
||||
|
||||
n_writers, n_rounds = 6, 15
|
||||
|
||||
def _writer(idx: int) -> None:
|
||||
for r in range(n_rounds):
|
||||
cache = SetupCache.empty(files)
|
||||
cache.normalizers[f"k{idx}"] = _entry(n_train_steps=r)
|
||||
setup_cache.save(data, files, cache)
|
||||
|
||||
threads = [threading.Thread(target=_writer, args=(i,)) for i in range(n_writers)]
|
||||
for t in threads:
|
||||
t.start()
|
||||
for t in threads:
|
||||
t.join()
|
||||
|
||||
loaded = setup_cache.load(data, files)
|
||||
assert loaded is not None
|
||||
assert set(loaded.normalizers.keys()) == {f"k{i}" for i in range(n_writers)}
|
||||
for i in range(n_writers):
|
||||
assert loaded.normalizers[f"k{i}"].n_train_steps == n_rounds - 1
|
||||
|
||||
|
||||
# ── energy_quantiles_from_sample / energy_quantile_at ───────────────────
|
||||
|
||||
|
||||
|
||||
@@ -1,9 +1,12 @@
|
||||
import warnings
|
||||
|
||||
import numpy as np
|
||||
import pytest
|
||||
from giant.constants import COND_DIM, COND_DIM_BASE, K_MAX
|
||||
from giant.data.transforms import (
|
||||
build_cond_features,
|
||||
build_features,
|
||||
encode_secondaries,
|
||||
energy_simplex_decode,
|
||||
energy_simplex_encode,
|
||||
inv_local_frame_rotation,
|
||||
@@ -24,6 +27,50 @@ def test_log_transform_invertible():
|
||||
np.testing.assert_allclose(inv_log_transform(log_transform(x)), x, rtol=1e-5)
|
||||
|
||||
|
||||
def test_log_transform_raises_on_input_below_negative_eps():
|
||||
"""A meaningfully negative input (upstream data corruption, not float
|
||||
noise near 0) must raise instead of silently returning NaN."""
|
||||
x = np.array([1.0, -5.0], dtype=np.float32)
|
||||
with np.errstate(invalid="ignore"), pytest.raises(ValueError, match="non-finite"):
|
||||
log_transform(x)
|
||||
|
||||
|
||||
def test_log_transform_raises_on_nan_input():
|
||||
x = np.array([1.0, np.nan], dtype=np.float32)
|
||||
with pytest.raises(ValueError, match="non-finite"):
|
||||
log_transform(x)
|
||||
|
||||
|
||||
def test_encode_secondaries_warns_when_sec_energies_exceed_e_sec():
|
||||
"""sec_E_list summing to more than e_sec (before the last slot is even
|
||||
reached) is a real upstream data mismatch — must warn instead of
|
||||
silently saturating the overflowing slot's stick-breaking logit via the
|
||||
_EPS floor. (A single slot alone exceeding what's left of the budget is
|
||||
the normal, expected "last slot takes the remainder" case and must NOT
|
||||
warn — the mismatch here is the *cumulative* sum through an earlier
|
||||
slot already exceeding e_sec.)"""
|
||||
sec_E_list = np.array([[5.0, 4.0, 1.0]], dtype=np.float32) # sums to 10
|
||||
sec_dir_list = np.tile([0.0, 0.0, 1.0], (1, 3, 1)).astype(np.float32)
|
||||
sec_valid = np.array([[True, True, True]])
|
||||
e_sec = np.array([6.0], dtype=np.float32) # cumsum already 9 by slot 2
|
||||
pre_dir = np.array([[0.0, 0.0, 1.0]], dtype=np.float32)
|
||||
|
||||
with pytest.warns(UserWarning, match="sec_E_list summing to more than e_sec"):
|
||||
encode_secondaries(sec_E_list, sec_dir_list, sec_valid, e_sec, pre_dir)
|
||||
|
||||
|
||||
def test_encode_secondaries_no_warning_when_energies_are_consistent():
|
||||
sec_E_list = np.array([[3.0, 2.0]], dtype=np.float32) # sums to 5
|
||||
sec_dir_list = np.tile([0.0, 0.0, 1.0], (1, 2, 1)).astype(np.float32)
|
||||
sec_valid = np.array([[True, True]])
|
||||
e_sec = np.array([6.0], dtype=np.float32) # >= 5, no shortfall
|
||||
pre_dir = np.array([[0.0, 0.0, 1.0]], dtype=np.float32)
|
||||
|
||||
with warnings.catch_warnings():
|
||||
warnings.simplefilter("error")
|
||||
encode_secondaries(sec_E_list, sec_dir_list, sec_valid, e_sec, pre_dir)
|
||||
|
||||
|
||||
def test_local_frame_rotation_noop_when_aligned():
|
||||
N = 8
|
||||
pre_dir = np.tile([0.0, 0.0, 1.0], (N, 1)).astype(np.float32)
|
||||
@@ -68,10 +115,43 @@ def test_local_frame_rotation_rejects_near_zero_pre_dir():
|
||||
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"):
|
||||
|
||||
|
||||
def test_local_frame_rotation_rejects_nan_pre_dir():
|
||||
"""A NaN pre_dir must raise loudly — `norm < 1e-6` is False for NaN, so
|
||||
without an explicit isfinite check this would silently poison the
|
||||
rotation (and any normalizer stats it feeds) instead of erroring."""
|
||||
pre_dir = np.array([[np.nan, 0.0, 1.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="non-finite"):
|
||||
local_frame_rotation(pre_dir, post_dir)
|
||||
with pytest.raises(ValueError, match="non-finite"):
|
||||
inv_local_frame_rotation(pre_dir, post_dir)
|
||||
|
||||
|
||||
def test_local_frame_rotation_antipodal_pre_dir_uses_x_axis_convention():
|
||||
"""pre_dir ~ -ẑ (near-exact backscatter) is a second axis_norm~0
|
||||
degeneracy besides pre_dir ~ +ẑ; unlike the forward case, the Rodrigues
|
||||
axis-dependent terms are NOT negligible there ((1-cos_t)~2), so the x̂
|
||||
fallback is a real (if arbitrary and physically rare) convention choice
|
||||
rather than a no-op. Pin it explicitly — angle-preservation and the
|
||||
round-trip property must still hold even though the "roll" is degenerate.
|
||||
"""
|
||||
pre_dir = np.array([[0.0, 0.0, -1.0]], dtype=np.float32)
|
||||
post_dir = np.array([[0.3, 0.4, 0.5]], dtype=np.float32)
|
||||
post_dir /= np.linalg.norm(post_dir, axis=1, keepdims=True)
|
||||
|
||||
rotated = local_frame_rotation(pre_dir, post_dir)
|
||||
|
||||
cos_before = (pre_dir * post_dir).sum(axis=1)
|
||||
cos_after = rotated[:, 2]
|
||||
np.testing.assert_allclose(cos_after, cos_before, atol=1e-5)
|
||||
np.testing.assert_allclose(np.linalg.norm(rotated, axis=1), 1.0, atol=1e-5)
|
||||
|
||||
recovered = inv_local_frame_rotation(pre_dir, rotated)
|
||||
np.testing.assert_allclose(recovered, post_dir, atol=1e-5)
|
||||
|
||||
|
||||
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."""
|
||||
|
||||
Reference in New Issue
Block a user