fix: OptimisationState limits guard, VariableData.any, redundant limits overrides

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

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}"

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:
         """

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
+        return -xp.log(self.upper_limit - self.lower_limit)

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)

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