From 754a20205e29d827c1218c27e7e0f8accabef9ae Mon Sep 17 00:00:00 2001 From: Jammy2211 Date: Fri, 10 Jul 2026 09:51:01 +0100 Subject: [PATCH] fix(priors): width-modifier safety + sigma validation agreement (#1346) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 2 of the #1331 priors/messages batch (Decisions 5 + 2, one PR by design so the width fix and the sigma check land together): - RelativeWidthModifier: sigma = value * abs(mean) (a negative posterior median previously produced a negative sigma that silently flipped the passed prior's scale) + opt-in absolute_floor (config round-trips via from_dict/dict; WidthModifier.__eq__ now compares full dicts) - Prior passing (mapper_from_prior_means, default width-modifier path): a computed width <= 0 (mean=0, relative modifier, no floor) raises a parameter-named PriorException with remediation guidance instead of silently freezing the parameter behind a sigma=0 delta prior. Explicit a=/r= widths keep their historical, test-pinned point-mass semantics. - NormalMessage: reject sigma < 0 (previously constructed silently with deceptive variance = sigma^2 > 0); the broken jax.lax.cond assert branch is retired (JAX defers to NaN propagation). sigma == 0 stays permitted: it is the established point-mass idiom (latent variables' simple_model_for_kwargs, from_mode(covariance=0), and model_centred_relative at mean=0 all depend on it) — evidence-based adjustment from the strict #1331 rec, disclosed on #1346. - TruncatedNormalMessage: unchanged sigma < 0 rejection with a clearer message — the two classes now agree, which was the actual defect. Adds test_prior_width_safety.py (11 regression tests incl. the mean=0 chained-parameter gate). Full suite: 1447 passed, 14 skipped. Co-Authored-By: Claude Opus 4.8 --- autofit/mapper/prior/width_modifier.py | 50 +++++++- autofit/mapper/prior_model/abstract.py | 14 +++ autofit/messages/normal.py | 49 ++++---- autofit/messages/truncated_normal.py | 6 +- .../mapper/prior/test_prior_width_safety.py | 114 ++++++++++++++++++ 5 files changed, 205 insertions(+), 28 deletions(-) create mode 100644 test_autofit/mapper/prior/test_prior_width_safety.py diff --git a/autofit/mapper/prior/width_modifier.py b/autofit/mapper/prior/width_modifier.py index 4d73d9d32..a4b29ba0b 100644 --- a/autofit/mapper/prior/width_modifier.py +++ b/autofit/mapper/prior/width_modifier.py @@ -22,9 +22,14 @@ def name_of_class(cls) -> str: @classmethod def from_dict(cls, width_modifier_dict): - return width_modifier_type_dict[width_modifier_dict["type"]]( - value=width_modifier_dict["value"] - ) + # Forward every key except "type" so optional keys (e.g. the + # RelativeWidthModifier absolute_floor) round-trip from config. + kwargs = { + key: value + for key, value in width_modifier_dict.items() + if key != "type" + } + return width_modifier_type_dict[width_modifier_dict["type"]](**kwargs) @abstractmethod def __call__(self, mean): @@ -72,12 +77,47 @@ def for_class_and_attribute_name(cls: type, attribute_name: str) -> "WidthModifi return RelativeWidthModifier(0.5) def __eq__(self, other): - return self.__class__ is other.__class__ and self.value == other.value + return self.__class__ is other.__class__ and self.dict == other.dict class RelativeWidthModifier(WidthModifier): + def __init__(self, value, absolute_floor=None): + """ + Prior-passing width proportional to the magnitude of the posterior + median: ``sigma = value * abs(mean)`` (#1331 Decision 5). + + ``abs`` guards the negative-median case, which previously produced a + negative sigma that flowed silently into the passed prior and flipped + its scale. For medians at (or very near) zero the relative width + collapses; set ``absolute_floor`` (here or via the ``width_modifier`` + entry in the priors config) to impose a minimum width. Without a floor, + a zero width is rejected loudly at prior-passing time. + + Parameters + ---------- + value + The proportionality constant applied to ``abs(mean)``. + absolute_floor + Optional minimum width. When set, the returned width is + ``max(value * abs(mean), absolute_floor)``. + """ + super().__init__(value) + self.absolute_floor = ( + float(absolute_floor) if absolute_floor is not None else None + ) + def __call__(self, mean): - return self.value * mean + sigma = self.value * abs(mean) + if self.absolute_floor is not None: + sigma = max(sigma, self.absolute_floor) + return sigma + + @property + def dict(self): + d = super().dict + if self.absolute_floor is not None: + d["absolute_floor"] = self.absolute_floor + return d class AbsoluteWidthModifier(WidthModifier): diff --git a/autofit/mapper/prior_model/abstract.py b/autofit/mapper/prior_model/abstract.py index 663d9fc69..1cc8d3074 100644 --- a/autofit/mapper/prior_model/abstract.py +++ b/autofit/mapper/prior_model/abstract.py @@ -1053,6 +1053,20 @@ def mapper_from_prior_means(self, means, a=None, r=None, no_limits=False): width = r * mean else: width = width_modifier(mean) + if width <= 0: + path = ".".join(map(str, self.path_for_prior(prior))) + raise exc.PriorException( + f"Prior passing produced a non-positive width " + f"(sigma={width}) for parameter '{path}' (posterior " + f"median mean={mean}). This happens when the posterior " + "median is zero and the parameter's width modifier is " + "relative with no floor. Configure an " + "AbsoluteWidthModifier or set absolute_floor on the " + "RelativeWidthModifier for this parameter in the " + "priors config. (Explicit a=/r= widths are exempt from " + "this check and keep their historical point-mass " + "semantics at mean=0.)" + ) if no_limits: limits = (float("-inf"), float("inf")) diff --git a/autofit/messages/normal.py b/autofit/messages/normal.py index f72b0133a..ecc72346b 100644 --- a/autofit/messages/normal.py +++ b/autofit/messages/normal.py @@ -24,25 +24,32 @@ def is_nan(value): return is_nan_ def assert_sigma_non_negative(sigma, xp=np): - - is_negative = sigma < 0 - - if xp.__name__.startswith("jax"): - import jax - # JAX path: cannot convert to Python bool - # Raise using JAX control flow: - return jax.lax.cond( - is_negative, - lambda _: (_ for _ in ()).throw( - ValueError("Sigma cannot be negative") - ), - lambda _: None, - operand=None, - ) - else: - # NumPy path: normal boolean works - if bool(is_negative): - raise ValueError("Sigma cannot be negative") + """ + Reject ``sigma < 0`` on the NumPy path (#1331 Decision 2), matching + ``TruncatedNormalMessage`` — the two classes previously disagreed, so a + negative sigma (e.g. from a negative posterior median through a relative + width modifier) constructed a ``NormalMessage`` silently and flipped the + scale of ``value_for`` / ``sample``. + + ``sigma == 0`` is deliberately permitted: it is the established point-mass + idiom — the latent-variables machinery wraps known values as + ``GaussianPrior(mean=value, sigma=0.0)`` (``non_linear/samples/util.py``), + ``from_mode(..., covariance=0)`` requests a point-mass projection, and + ``model_centred_relative`` pins ``sigma == 0`` for zero-centred parameters. + + The JAX path is a deliberate no-op: a traced ``sigma`` cannot be converted + to a Python bool, so validity defers to NaN propagation. (The previous + ``jax.lax.cond`` branch here never worked — ``lax.cond`` traces both + branches, so the generator-throw lambda was suppressed under ``jit``.) + """ + if xp is np: + if (np.asarray(sigma) < 0).any(): + raise exc.MessageException( + f"NormalMessage sigma cannot be negative, got sigma={sigma}. " + "Negative widths typically come from prior passing with a " + "parameter whose posterior median is negative — see " + "RelativeWidthModifier in the priors config." + ) class NormalMessage(AbstractMessage): @@ -84,7 +91,7 @@ def __init__( The mean (μ) of the normal distribution. sigma - The standard deviation (σ) of the distribution. Must be non-negative. + The standard deviation (σ) of the distribution. Must be strictly positive. log_norm An additive constant to the log probability of the message. Used internally for message-passing normalization. @@ -100,7 +107,7 @@ def __init__( import jax.numpy as jnp xp = jnp - # assert_sigma_non_negative(sigma, xp=xp) + assert_sigma_non_negative(sigma, xp=xp) super().__init__( mean, diff --git a/autofit/messages/truncated_normal.py b/autofit/messages/truncated_normal.py index 9915e1bff..10ac3bd1d 100644 --- a/autofit/messages/truncated_normal.py +++ b/autofit/messages/truncated_normal.py @@ -88,7 +88,7 @@ def __init__( mean The mean (μ) of the normal distribution. sigma - The standard deviation (σ) of the distribution. Must be non-negative. + The standard deviation (σ) of the distribution. Must be strictly positive. log_norm An additive constant to the log probability of the message. Used internally for message-passing normalization. Default is 0.0. @@ -96,7 +96,9 @@ def __init__( An optional unique identifier used to track the message in larger probabilistic graphs or models. """ if (np.array(sigma) < 0).any(): - raise exc.MessageException("Sigma cannot be negative") + raise exc.MessageException( + f"TruncatedNormalMessage sigma cannot be negative, got sigma={sigma}." + ) super().__init__( mean, diff --git a/test_autofit/mapper/prior/test_prior_width_safety.py b/test_autofit/mapper/prior/test_prior_width_safety.py new file mode 100644 index 000000000..255bcc2ce --- /dev/null +++ b/test_autofit/mapper/prior/test_prior_width_safety.py @@ -0,0 +1,114 @@ +"""Regression tests for the width-modifier safety pair (PyAutoFit #1346, +Phase 2 of decision hub #1331 — Decisions 5 + 2). Numpy-only. +""" +import pytest + +import autofit as af +from autofit import exc +from autofit.mapper.prior.width_modifier import ( + RelativeWidthModifier, + WidthModifier, +) +from autofit.messages.normal import NormalMessage +from autofit.messages.truncated_normal import TruncatedNormalMessage + + +# --- Decision 5: RelativeWidthModifier uses abs(mean), optional absolute_floor --- + +def test__relative_width_modifier_abs_mean(): + mod = RelativeWidthModifier(0.5) + # A negative posterior median previously produced a negative sigma that + # flowed silently into the passed prior and flipped its scale. + assert mod(-2.0) == 1.0 + assert mod(2.0) == 1.0 + # The bare modifier still returns 0.0 at mean=0; prior passing rejects it + # loudly downstream (see the chained tests below). + assert mod(0.0) == 0.0 + + +def test__relative_width_modifier_floor(): + mod = RelativeWidthModifier(0.5, absolute_floor=0.1) + assert mod(0.0) == 0.1 # floor engages at zero median + assert mod(0.1) == 0.1 # 0.5 * 0.1 = 0.05 < floor + assert mod(10.0) == 5.0 # a floor, not a cap + + +def test__relative_width_modifier_dict_round_trip(): + mod = RelativeWidthModifier(0.5, absolute_floor=0.1) + assert mod.dict == {"type": "Relative", "value": 0.5, "absolute_floor": 0.1} + assert WidthModifier.from_dict(mod.dict) == mod + + bare = RelativeWidthModifier(0.5) + assert bare.dict == {"type": "Relative", "value": 0.5} + assert WidthModifier.from_dict(bare.dict) == bare + assert bare != mod + + +# --- Decision 2 (evidence-adjusted): both message classes now agree — sigma < 0 +# rejected, sigma == 0 permitted as the established point-mass idiom (latent +# variables' simple_model_for_kwargs, from_mode(covariance=0), +# model_centred_relative at mean=0 all depend on it) --- + +def test__normal_message_rejects_negative_sigma(): + with pytest.raises(exc.MessageException): + NormalMessage(mean=0.0, sigma=-1.0) + + +def test__truncated_normal_message_rejects_negative_sigma(): + with pytest.raises(exc.MessageException): + TruncatedNormalMessage( + mean=0.0, sigma=-1.0, lower_limit=-1.0, upper_limit=1.0 + ) + + +def test__sigma_zero_point_mass_still_constructs(): + # The point-mass carrier used by the latent-variables machinery + # (non_linear/samples/util.py) and from_mode(covariance=0) must keep working. + m = NormalMessage(mean=3.0, sigma=0.0) + assert m.sigma == 0.0 + p = af.GaussianPrior(mean=3.0, sigma=0.0) + assert p.sigma == 0.0 + + +def test__gaussian_prior_rejects_negative_sigma(): + # Previously constructed silently with deceptive variance = sigma**2 > 0 + # and a sign-flipped value_for. + with pytest.raises(exc.MessageException): + af.GaussianPrior(mean=0.0, sigma=-0.5) + + +# --- The mean=0 chained-parameter regression (the Phase-2 sequencing gate: +# yesterday's silent delta-freeze must become a clear, parameter-named error, +# and the floor must be the working remedy) --- + +def _mapper_with_relative_widths(absolute_floor=None): + mapper = af.ModelMapper(mock_class=af.m.MockClassx2) + for prior in mapper.priors: + prior.width_modifier = RelativeWidthModifier( + 0.5, absolute_floor=absolute_floor + ) + return mapper + + +def test__prior_passing_mean_zero_raises_with_guidance(): + mapper = _mapper_with_relative_widths() + with pytest.raises(exc.PriorException) as err: + mapper.mapper_from_prior_means([0.0, 5.0]) + # The error must name the parameter and point at the remedy. + assert "mock_class" in str(err.value) + assert "absolute_floor" in str(err.value) + + +def test__prior_passing_mean_zero_with_floor_passes(): + mapper = _mapper_with_relative_widths(absolute_floor=0.1) + result = mapper.mapper_from_prior_means([0.0, 5.0]) + assert result.mock_class.one.mean == 0.0 + assert result.mock_class.one.sigma == 0.1 # floor engaged + assert result.mock_class.two.sigma == 2.5 # 0.5 * 5.0, floor irrelevant + + +def test__prior_passing_negative_mean_gets_positive_width(): + mapper = _mapper_with_relative_widths() + result = mapper.mapper_from_prior_means([-2.0, 5.0]) + assert result.mock_class.one.mean == -2.0 + assert result.mock_class.one.sigma == 1.0 # 0.5 * abs(-2.0)