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