From 4c0f79bac867f1349dbc9cfa3fc216388d7d71c9 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 17:12:03 +0000 Subject: [PATCH] fix: OptimisationState limits guard, VariableData.any, redundant limits overrides MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three items left behind by PyAutoFit#1527, plus one live bug found while doing the first. VariableData.any dispatched through var_all ------------------------------------------- `VariableData.any` reduced via `var_all`, making it "is there a variable whose elements are ALL True" rather than "is ANY element True". For an array like `[True, False]` it answered False. That is a correctness bug in `OptimisationState.valid`, which asks `(parameters < lower_limit).any()`: a parameter vector with SOME components outside their limits was reported valid, and only a variable violating on EVERY component was caught. `MeanField`'s `valid.any()` under-reported the same way. Both call sites want a real `any`; the other four `.any()` call sites in the library are on numpy arrays and are untouched. OptimisationState.valid guards on truthiness -------------------------------------------- `if self.lower_limit and ...` now `is not None`. `lower_limit`/`upper_limit` are VariableData (a dict keyed by free variable) or None, so the old form worked only by accident of dict truthiness being non-emptiness. #1527's follow-up list read it as a scalar test and recorded a `0.0` bug that does not exist — but the guard was one type change away from making it real. Behaviour-preserving: the only case whose guard differs is the empty VariableData, where the comparison is empty and `.any()` is False either way. Redundant limits overrides removed ---------------------------------- #1527 made `Prior.limits` derive from `lower_limit`/`upper_limit`, leaving the `UniformPrior`, `LogUniformPrior` and `TruncatedGaussianPrior` overrides exact duplicates. The base coerces with `float()` and the overrides did not, so this was a possible type change — measured, and it is not one: all three store Python floats already, and under `jax.jit` a prior never reaches `limits` anyway (`tree_unflatten` -> `__init__` calls `float()` on the tracer and raises first, with or without this change). Now-unused `Tuple` imports dropped. Tests ----- `OptimisationState.valid` and `VariableData.any` had NO test coverage, so the suite would have stayed green through any change to either — the process lesson from #1477. New test_autofit/graphical/test_optimisation_state_valid.py covers both, verified by inversion: reverting `var_any` to `var_all` fails 3 of the 8. Full suite: 2186 passed, 36 skipped (baseline 2178/36, +8 new). Co-Authored-By: Claude --- autofit/graphical/laplace/line_search.py | 17 ++- autofit/mapper/prior/log_uniform.py | 6 +- autofit/mapper/prior/truncated_gaussian.py | 6 +- autofit/mapper/prior/uniform.py | 9 +- autofit/mapper/variable.py | 7 +- .../test_optimisation_state_valid.py | 142 ++++++++++++++++++ 6 files changed, 167 insertions(+), 20 deletions(-) create mode 100644 test_autofit/graphical/test_optimisation_state_valid.py diff --git a/autofit/graphical/laplace/line_search.py b/autofit/graphical/laplace/line_search.py index fb3c097b6..2ad1092f5 100644 --- a/autofit/graphical/laplace/line_search.py +++ b/autofit/graphical/laplace/line_search.py @@ -105,10 +105,23 @@ def __init__( @property def valid(self): - if self.lower_limit and (self.parameters < self.lower_limit).any(): + """ + Whether the current parameters lie inside the limits, when limits are set. + + ``lower_limit`` / ``upper_limit`` are ``VariableData`` (a ``dict`` keyed by + free variable), supplied by ``LaplaceOptimiser`` only when + ``check_limits=True`` and ``None`` otherwise. The guards test ``is not None`` + rather than truthiness: the intent is "was a limit supplied?", and ``if x`` + expresses that only by accident of ``VariableData`` being a ``dict``, whose + truthiness is non-emptiness. It reads as a scalar test — PyAutoFit#1527's + follow-up list recorded it as one, as a `0.0` bug that does not exist — and + would become one on any change of type: a scalar would make ``0.0`` skip the + check, and a bare array would raise on the ambiguous truth value. + """ + if self.lower_limit is not None and (self.parameters < self.lower_limit).any(): return False - if self.upper_limit and (self.parameters > self.upper_limit).any(): + if self.upper_limit is not None and (self.parameters > self.upper_limit).any(): return False return True diff --git a/autofit/mapper/prior/log_uniform.py b/autofit/mapper/prior/log_uniform.py index 8ae4e7ed4..e719f8470 100644 --- a/autofit/mapper/prior/log_uniform.py +++ b/autofit/mapper/prior/log_uniform.py @@ -1,4 +1,4 @@ -from typing import Optional, Tuple +from typing import Optional import numpy as np @@ -195,10 +195,6 @@ def dict(self) -> dict: prior_dict = super().dict() return {**prior_dict, "lower_limit": self.lower_limit, "upper_limit": self.upper_limit} - @property - def limits(self) -> Tuple[float, float]: - return self.lower_limit, self.upper_limit - @property def parameter_string(self) -> str: return f"lower_limit = {self.lower_limit}, upper_limit = {self.upper_limit}" diff --git a/autofit/mapper/prior/truncated_gaussian.py b/autofit/mapper/prior/truncated_gaussian.py index 20a07ce9a..2ed809ceb 100644 --- a/autofit/mapper/prior/truncated_gaussian.py +++ b/autofit/mapper/prior/truncated_gaussian.py @@ -1,4 +1,4 @@ -from typing import Optional, Tuple +from typing import Optional import numpy as np @@ -118,10 +118,6 @@ def dict(self) -> dict: "upper_limit": self.upper_limit } - @property - def limits(self) -> Tuple[float, float]: - return self.lower_limit, self.upper_limit - @property def parameter_string(self) -> str: """ diff --git a/autofit/mapper/prior/uniform.py b/autofit/mapper/prior/uniform.py index 4390cdf43..9afd206d5 100644 --- a/autofit/mapper/prior/uniform.py +++ b/autofit/mapper/prior/uniform.py @@ -1,5 +1,5 @@ import numpy as np -from typing import Optional, Tuple +from typing import Optional from autofit.messages.normal import UniformNormalMessage from .abstract import Prior @@ -199,9 +199,4 @@ def log_prior_from_value(self, value, xp=np): def log_normalisation(self, xp=np) -> float: """The constant ``-log(upper - lower)`` dropped from ``log_prior_from_value`` (which returns ``0.0``). See ``Prior.log_normalisation``.""" - return -xp.log(self.upper_limit - self.lower_limit) - - @property - def limits(self) -> Tuple[float, float]: - """The (lower_limit, upper_limit) bounds of this uniform prior.""" - return self.lower_limit, self.upper_limit \ No newline at end of file + return -xp.log(self.upper_limit - self.lower_limit) \ No newline at end of file diff --git a/autofit/mapper/variable.py b/autofit/mapper/variable.py index 0f2266de2..f2ed79c4d 100644 --- a/autofit/mapper/variable.py +++ b/autofit/mapper/variable.py @@ -394,7 +394,12 @@ def all(self) -> bool: return all(VariableData.var_all(self).values()) def any(self) -> bool: - return any(VariableData.var_all(self).values()) + # ``var_any``, not ``var_all``: this reduces "is ANY element True", + # and dispatching it through ``var_all`` made it "is there a variable + # whose elements are ALL True". For a partially-violating array that + # answered False, so ``OptimisationState.valid`` accepted parameter + # vectors with some components outside their limits. + return any(VariableData.var_any(self).values()) def det(self) -> float: return VariableData.var_det(self).reduce(operator.mul) diff --git a/test_autofit/graphical/test_optimisation_state_valid.py b/test_autofit/graphical/test_optimisation_state_valid.py new file mode 100644 index 000000000..13c3e5109 --- /dev/null +++ b/test_autofit/graphical/test_optimisation_state_valid.py @@ -0,0 +1,142 @@ +""" +``OptimisationState.valid`` — the limits guard on the Laplace line search. + +Written alongside the guard's rewrite from ``if self.lower_limit`` to +``if self.lower_limit is not None``. The property had **no test at all** before +this file, so the suite would have stayed green through any change to it — the +process lesson recorded in ``prior-support-clipper`` (PyAutoFit#1477), where a +1790-test suite passed against an ``LBFGS._fit`` that raised ``NameError`` on +every real call. + +The guards test ``is not None`` rather than truthiness because the intent is +"was a limit supplied?". ``lower_limit`` / ``upper_limit`` are ``VariableData`` +(a ``dict`` keyed by free variable) or ``None``; ``if x`` expressed that only by +accident of dict truthiness being non-emptiness. +""" + +import numpy as np +import pytest + +from autofit.graphical.laplace.line_search import OptimisationState +from autofit.mapper.variable import Variable, VariableData + + +@pytest.fixture(name="x") +def make_variable(): + return Variable("x") + + +def _state(parameters, lower_limit=None, upper_limit=None): + """ + An ``OptimisationState`` carrying only what ``valid`` reads. + + ``__init__`` evaluates ``self.valid`` and, when invalid, replaces value and + gradient with ``inf`` — which is why the factor callables below must be + tolerated rather than invoked. ``valid`` itself touches none of them. + """ + return OptimisationState( + factor=lambda *_: None, + factor_gradient=lambda *_: None, + parameters=parameters, + lower_limit=lower_limit, + upper_limit=upper_limit, + ) + + +def test__no_limits_supplied__always_valid(x): + """``check_limits=False`` leaves both limits ``None``; nothing is enforced.""" + state = _state(VariableData({x: np.array([-1e9, 1e9])})) + + assert state.lower_limit is None + assert state.upper_limit is None + assert state.valid is True + + +def test__empty_limits__valid(x): + """ + The case the old truthiness guard and the new ``is not None`` guard resolve + differently *in the guard*, and identically *in the answer*. + + An empty ``VariableData`` is falsy, so the old guard skipped the comparison; + the new guard runs it, and an empty comparison's ``.any()`` is ``False``. A + model with no free variables is valid either way — this pins that the rewrite + did not change it. + """ + state = _state( + VariableData({x: np.array([-5.0])}), + lower_limit=VariableData({}), + upper_limit=VariableData({}), + ) + + assert state.valid is True + + +def test__inside_the_limits__valid(x): + state = _state( + VariableData({x: np.array([0.5, 1.5])}), + lower_limit=VariableData({x: np.array([0.0, 0.0])}), + upper_limit=VariableData({x: np.array([2.0, 2.0])}), + ) + + assert state.valid is True + + +@pytest.mark.parametrize( + "parameters, expected", + [ + (np.array([-0.1, 1.0]), False), # below the lower limit + (np.array([1.0, 2.1]), False), # above the upper limit + (np.array([0.0, 2.0]), True), # exactly on both — inclusive + ], +) +def test__limits_are_enforced_and_inclusive(x, parameters, expected): + state = _state( + VariableData({x: parameters}), + lower_limit=VariableData({x: np.array([0.0, 0.0])}), + upper_limit=VariableData({x: np.array([2.0, 2.0])}), + ) + + assert state.valid is expected + + +def test__a_zero_lower_limit_is_still_enforced(x): + """ + The regression the rewrite exists for. + + Under the old ``if self.lower_limit`` guard this is safe only because + ``VariableData`` is a ``dict``. PyAutoFit#1527's follow-up list read the guard + as a scalar test and recorded a `0.0` bug that did not exist — but the guard + was one type change away from making it real. Pinning a lower limit of + exactly ``0.0`` keeps that honest: a scalar-valued limit of ``0.0`` must + never mean "no limit". + """ + state = _state( + VariableData({x: np.array([-1.0])}), + lower_limit=VariableData({x: np.array([0.0])}), + ) + + assert state.valid is False + + +# === VariableData.any === + + +def test__variable_data_any_reduces_any_not_all(x): + """ + ``VariableData.any`` dispatched through ``var_all``, which made it "is there a + variable whose elements are ALL True" rather than "is ANY element True". + + This is why ``OptimisationState.valid`` above could not see a *partial* + violation: for ``array([True, False])`` the old reduction answered ``False``, + so a parameter vector with one component outside its limits was accepted. The + guard rewrite alone does not fix that — this does. + """ + assert VariableData({x: np.array([True, False])}).any() is True + assert VariableData({x: np.array([False, False])}).any() is False + assert VariableData({x: np.array([True, True])}).any() is True + assert VariableData({}).any() is False + + # ``all`` is the counterpart and was always correct; pinned so a future + # edit cannot "fix" one by breaking the other. + assert VariableData({x: np.array([True, False])}).all() is False + assert VariableData({x: np.array([True, True])}).all() is True