diff --git a/changelog.d/fix-metrics-denominator.fixed.md b/changelog.d/fix-metrics-denominator.fixed.md new file mode 100644 index 0000000..593fc2a --- /dev/null +++ b/changelog.d/fix-metrics-denominator.fixed.md @@ -0,0 +1 @@ +Guard the `(target + 1)` denominator in loss() and pct_close() against targets equal to -1 (divide-by-zero -> Inf) and extend the guard in loss() to reject non-finite rel_error (not just NaN). For any target with target+1 comfortably positive the numeric result is unchanged. diff --git a/src/microcalibrate/utils/metrics.py b/src/microcalibrate/utils/metrics.py index 37237ad..7e9b5a3 100644 --- a/src/microcalibrate/utils/metrics.py +++ b/src/microcalibrate/utils/metrics.py @@ -5,6 +5,22 @@ import torch +def _safe_denominator( + targets_array: torch.Tensor, eps: float = 1e-12 +) -> torch.Tensor: + """Return a strictly-positive denominator for the (est-t)/(t+1) style + relative-error metrics. + + The original formulation used ``targets_array + 1`` which crosses + zero at ``target == -1`` (producing Inf) and is nearly zero for any + target in a small neighbourhood. Clamping by a small ``eps`` + preserves the exact value for all well-behaved targets + (``target + 1 >= eps``) while guaranteeing finite output when a + target happens to equal ``-1``. + """ + return torch.clamp(targets_array + 1, min=eps) + + def loss( estimate: torch.Tensor, targets_array: torch.Tensor, @@ -20,11 +36,16 @@ def loss( Returns: torch.Tensor: Mean squared relative error between estimated and target values. """ - rel_error = (((estimate - targets_array) + 1) / (targets_array + 1)) ** 2 + denominator = _safe_denominator(targets_array) + rel_error = (((estimate - targets_array) + 1) / denominator) ** 2 if normalization_factor is not None: rel_error *= normalization_factor - if torch.isnan(rel_error).any(): - raise ValueError("Relative error contains NaNs") + if not torch.isfinite(rel_error).all(): + raise ValueError( + "Relative error contains non-finite values (NaN or Inf). " + "Check the inputs: targets near -1, zero initial weights, " + "or Inf/NaN from the estimate function can cause this." + ) return rel_error.mean() @@ -43,5 +64,6 @@ def pct_close( Returns: float: Percentage of estimates within the threshold. """ - abs_error = torch.abs((estimate - targets) / (1 + targets)) + denominator = _safe_denominator(targets) + abs_error = torch.abs((estimate - targets) / denominator) return ((abs_error < t).sum() / abs_error.numel()).item() diff --git a/tests/test_metrics_regression.py b/tests/test_metrics_regression.py new file mode 100644 index 0000000..4abc304 --- /dev/null +++ b/tests/test_metrics_regression.py @@ -0,0 +1,54 @@ +"""Regression tests for the +1 smoothing bug in loss/pct_close +(finding #7). + +The original `((estimate - target) + 1) / (target + 1)` construction +produced Inf when ``target == -1`` and the NaN guard only caught NaN, +so calibration silently diverged. After the fix the denominator is +clamped to be strictly positive and the guard rejects any non-finite +rel_error. +""" + +import math + +import pytest +import torch + +from microcalibrate.utils.metrics import loss, pct_close + + +def test_loss_raises_on_target_equal_to_neg_one() -> None: + """A target value of exactly -1 used to produce Inf silently; the + guard must reject non-finite rel_error instead of NaN-only.""" + estimate = torch.tensor([0.0, 0.0], dtype=torch.float32) + targets = torch.tensor([-1.0, 1.0], dtype=torch.float32) + l = loss(estimate, targets) + # With the clamp in place the result is finite; check that. + assert torch.isfinite(l) + + +def test_loss_rejects_non_finite_rel_error() -> None: + """If the estimate itself is non-finite (e.g. an upstream NaN), + ``loss`` must raise -- the old ``torch.isnan`` check missed Inf.""" + estimate = torch.tensor([float("inf"), 0.0, 0.0], dtype=torch.float32) + targets = torch.tensor([1.0, 2.0, 3.0], dtype=torch.float32) + with pytest.raises(ValueError): + loss(estimate, targets) + + +def test_loss_preserves_values_for_well_behaved_targets() -> None: + """For targets where target+1 is comfortably positive, the loss + must equal the original formula (backwards compatibility).""" + estimate = torch.tensor([1.0, 2.0, 3.0], dtype=torch.float32) + targets = torch.tensor([0.5, 1.5, 2.5], dtype=torch.float32) + expected = (((estimate - targets) + 1) / (targets + 1)) ** 2 + assert torch.allclose(loss(estimate, targets), expected.mean()) + + +def test_pct_close_finite_on_target_equal_to_neg_one() -> None: + """pct_close used the same (1 + target) denominator; it must not + return NaN/Inf when ``target == -1``.""" + estimate = torch.tensor([0.0, 0.0], dtype=torch.float32) + targets = torch.tensor([-1.0, 1.0], dtype=torch.float32) + result = pct_close(estimate, targets) + assert math.isfinite(result) + assert 0.0 <= result <= 1.0