Skip to content

Commit c3505d1

Browse files
committed
fix: LogGaussianPrior declares its own (0, inf) support
`LogGaussianPrior`'s support is `(0, inf)` -- `log_prior_from_value` returns `-inf` for `value <= 0` -- but the prior reported `(-inf, inf)`, because `Prior.__getattr__` delegated to a `TransformedMessage` whose limits default to `+/-inf` and were never set. `ClipperPriorBox` worked around it with an `isinstance` switch (PyAutoFit#1477, follow-up 3); every other consumer of `lower_limit` was simply told the wrong thing. Declare the support on the prior itself, and give `Prior` a general strictness contract so a consumer can tell an exclusive bound from an inclusive one without a type switch: - `Prior.lower_limit_strict` / `upper_limit_strict`, class attributes defaulting to `False`; `True` for `LogGaussianPrior`'s lower bound. - `LogGaussianPrior` sets `lower_limit = 0.0` / `upper_limit = inf` in `__init__`. - `Prior.limits` derives from `lower_limit`/`upper_limit` instead of returning a hardcoded `(-inf, inf)`, so the two notions cannot disagree by construction. - `ClipperPriorBox._limits_from_model` reads the strictness flags; its `isinstance` block, import and workaround docstrings are retired. The limits go on the prior rather than on the `TransformedMessage` deliberately. On the message they would be dropped by `with_base`, `copy`, `project` and `__call__`, and would change what `MeanField.lower_limit` and `LaplaceOptimiser(check_limits=True)` feed to `OptimisationState.valid` -- a live EP behaviour change well outside this fix. Verified before/after over 37 behavioural probes: identifiers (prior and search-with-clipper) byte-identical, so no output directory re-keys; `log_prior_from_value` pointwise identical; the nested-sampler unit-cube mapping identical; clipper bounds, projections and masks identical across 3 clipper configurations x 3 input vectors. One downstream-visible change: prior passing on a LogGaussian parameter with no config `Limits` entry previously produced `TruncatedGaussian(-inf, inf)`, whose `value_for(0.001)` was `-0.545` and whose `log_prior_from_value(-1.0)` was a finite `-8.0`. It is now lower-bounded at `0.0`. Tests: the reported support, the strictness flags, regression pins for `log_prior_from_value` / the unit-cube mapping / the identifier, and a parametrised property over every prior family asserting that a prior's reported support matches its actual support -- the general form of this bug, which fails on the parent commit for `LogGaussianPrior` alone. Closes #1526 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AXEqAUkXK7UeXw5jGh2i4q
1 parent 9ddf25c commit c3505d1

6 files changed

Lines changed: 276 additions & 34 deletions

File tree

autofit/mapper/prior/abstract.py

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,20 @@ class Prior(Variable, ABC, ArithmeticMixin):
2121

2222
_ids = itertools.count()
2323

24+
#: Whether the support *excludes* the corresponding limit itself, i.e. whether
25+
#: the bound is ``value > limit`` rather than ``value >= limit``.
26+
#:
27+
#: Most priors are inclusive at their limits: ``UniformPrior.log_prior_from_value``
28+
#: is finite exactly on the bound. ``LogGaussianPrior`` is not — its support is the
29+
#: open ``(0, inf)`` — and a consumer that clips onto an exclusive bound lands on a
30+
#: point of zero density. These flags let such a consumer tell the two apart without
31+
#: a per-type ``isinstance`` switch (see ``non_linear.clipper.ClipperPriorBox``).
32+
#:
33+
#: Class attributes, deliberately: they are a property of the prior *family*, never
34+
#: of an instance, so they stay out of ``__dict__`` and out of the identifier.
35+
lower_limit_strict = False
36+
upper_limit_strict = False
37+
2438
def __init__(self, message, id_=None):
2539
"""
2640
An object used to mappers a unit value to an attribute value for a specific
@@ -347,10 +361,16 @@ def name_of_class(cls) -> str:
347361
def limits(self) -> Tuple[float, float]:
348362
"""The (lower, upper) bounds of this prior.
349363
350-
Returns (-inf, inf) by default. Subclasses with finite bounds
351-
(e.g. UniformPrior) override this.
364+
Derived from ``lower_limit`` / ``upper_limit`` rather than stated
365+
separately, so the two cannot disagree. They used to: this property
366+
returned a hardcoded ``(-inf, inf)`` for every prior that did not
367+
override it, which was right for ``GaussianPrior`` and wrong for
368+
``LogGaussianPrior``, whose support is ``(0, inf)``.
369+
370+
Use ``lower_limit_strict`` / ``upper_limit_strict`` to tell whether the
371+
support includes the bounds returned here.
352372
"""
353-
return (float("-inf"), float("inf"))
373+
return (float(self.lower_limit), float(self.upper_limit))
354374

355375
def gaussian_prior_model_for_arguments(self, arguments):
356376
"""Look up this prior in an arguments dict and return the mapped value.

autofit/mapper/prior/log_gaussian.py

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,10 @@ class LogGaussianPrior(Prior):
1212
__identifier_fields__ = ("mean", "sigma")
1313
__database_args__ = ("mean", "sigma", "id_")
1414

15+
#: The support is the *open* interval ``(0, inf)``: ``log_prior_from_value``
16+
#: returns ``-inf`` at ``0`` itself, so a consumer must stay strictly above it.
17+
lower_limit_strict = True
18+
1519
def __init__(
1620
self,
1721
mean: float,
@@ -51,6 +55,15 @@ def __init__(
5155
self.mean = mean
5256
self.sigma = sigma
5357

58+
# Declared on the prior, not on the message below. `TransformedMessage`
59+
# defaults its limits to +/-inf and derives its `_support` separately, so
60+
# a prior that left them to delegation reported (-inf, inf) for a strictly
61+
# positive parameter -- the bug this shadowing fixes. Keeping the message's
62+
# own limits untouched keeps the EP/Laplace machinery, which reads them,
63+
# behaving exactly as before.
64+
self.lower_limit = 0.0
65+
self.upper_limit = float("inf")
66+
5467
message = TransformedMessage(
5568
NormalMessage(mean, sigma),
5669
log_transform,

autofit/non_linear/clipper.py

Lines changed: 19 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -99,8 +99,6 @@
9999

100100
import numpy as np
101101

102-
from autofit.mapper.prior.log_gaussian import LogGaussianPrior
103-
104102
logger = logging.getLogger(__name__)
105103

106104

@@ -225,7 +223,9 @@ class ClipperPriorBox(AbstractClipper):
225223
``(0, inf)``) — inset by an *absolute* ``strict_epsilon``. A relative margin
226224
is identically zero here, there being no finite width, so it would clip
227225
exactly onto ``0.0``, where the support is strict and ``log_prior`` is
228-
``-inf``. Only an absolute nudge lands strictly inside.
226+
``-inf``. Only an absolute nudge lands strictly inside. Which bounds are
227+
exclusive is read off the prior's ``lower_limit_strict`` /
228+
``upper_limit_strict``, never inferred from its type.
229229
230230
Parameters
231231
----------
@@ -249,23 +249,20 @@ def _limits_from_model(self, model):
249249
The raw ``(lower, upper, lower_strict, upper_strict)`` arrays for ``model``,
250250
in physical parameter order.
251251
252-
Limits are read off ``prior.lower_limit`` / ``prior.upper_limit``, which
253-
resolves for **every** prior type without a type switch: ``Prior.__getattr__``
254-
delegates to the prior's message, and ``AbstractMessage`` defaults both to
255-
``±inf``. ``UniformPrior`` and ``LogUniformPrior`` shadow them with their
256-
own attributes, ``TruncatedGaussianPrior`` picks up real limits from
257-
``TruncatedNormalMessage``, and ``GaussianPrior`` correctly falls through
258-
to ``±inf``.
259-
260-
``LogGaussianPrior`` is the one prior that read gets *wrong*, and silently.
261-
Its message is a ``TransformedMessage``, which defaults its limits to
262-
``±inf`` and is never passed any — yet
263-
``LogGaussianPrior.log_prior_from_value`` returns ``-inf`` for
264-
``value <= 0``. Left uncorrected, the clipper would report that coordinate
265-
as unbounded and fail to protect precisely the mechanism it exists to fix,
266-
so its ``(0, inf)`` support is declared here. Declaring it on the prior
267-
itself is the cleaner fix, but it would change a class the EP machinery and
268-
the nested samplers also read, so it is deliberately kept local.
252+
Limits are read off ``prior.lower_limit`` / ``prior.upper_limit``, and
253+
strictness off ``prior.lower_limit_strict`` / ``prior.upper_limit_strict``.
254+
Both resolve for **every** prior type without a type switch: ``UniformPrior``
255+
and ``LogUniformPrior`` shadow the limits with their own attributes,
256+
``LogGaussianPrior`` declares its ``(0, inf)`` support and flags the lower
257+
bound strict, ``TruncatedGaussianPrior`` picks up real limits from
258+
``TruncatedNormalMessage``, and ``GaussianPrior`` falls through
259+
``Prior.__getattr__`` to its message's ``±inf``.
260+
261+
This clipper used to declare ``LogGaussianPrior``'s support itself, because
262+
that prior reported ``(-inf, inf)`` while ``log_prior_from_value`` returned
263+
``-inf`` for ``value <= 0``. That was a workaround for a defect in the prior,
264+
fixed in PyAutoFit#1526: any consumer of ``lower_limit`` — not just this one —
265+
was being told a strictly positive parameter could go negative.
269266
270267
The ``strict`` flags mark bounds the support *excludes* (``value > limit``
271268
rather than ``value >= limit``); only those need the absolute inset.
@@ -275,12 +272,8 @@ def _limits_from_model(self, model):
275272
for prior in model.priors_ordered_by_id:
276273
low = float(prior.lower_limit)
277274
high = float(prior.upper_limit)
278-
low_strict = False
279-
high_strict = False
280-
281-
if isinstance(prior, LogGaussianPrior):
282-
low = 0.0
283-
low_strict = True
275+
low_strict = bool(prior.lower_limit_strict)
276+
high_strict = bool(prior.upper_limit_strict)
284277

285278
lower.append(low)
286279
upper.append(high)

test_autofit/mapper/prior/test_log_gaussian.py

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
import pickle
2+
from copy import copy
23

4+
import numpy as np
35
import pytest
46

57
import autofit as af
@@ -30,3 +32,129 @@ def test_pickle(log_gaussian):
3032

3133
def test_identifier(log_gaussian):
3234
Identifier(log_gaussian)
35+
36+
37+
# === PyAutoFit#1526: the prior declares its own (0, inf) support ===
38+
#
39+
# The support was always (0, inf) -- ``log_prior_from_value`` returns -inf for
40+
# ``value <= 0`` -- but the prior reported (-inf, inf), because ``Prior.__getattr__``
41+
# delegated to a ``TransformedMessage`` whose limits default to +/-inf and were never
42+
# set. ``ClipperPriorBox`` worked around it with an ``isinstance`` switch; every other
43+
# consumer of ``lower_limit`` was simply told the wrong thing.
44+
#
45+
# The pinned values below are the pre-change ones, measured on the commit before the
46+
# fix. They are the guarantee that declaring the support moved *what the prior says*
47+
# and nothing about *what it computes*.
48+
49+
50+
def test__reports_its_own_support(log_gaussian):
51+
assert log_gaussian.lower_limit == 0.0
52+
assert log_gaussian.upper_limit == float("inf")
53+
assert log_gaussian.limits == (0.0, float("inf"))
54+
55+
56+
def test__lower_bound_is_strict__upper_is_not(log_gaussian):
57+
"""
58+
The support is the *open* (0, inf): ``log_prior_from_value(0.0)`` is -inf, so a
59+
consumer that clips onto the reported bound must know to stay strictly above it.
60+
"""
61+
assert log_gaussian.lower_limit_strict is True
62+
assert log_gaussian.upper_limit_strict is False
63+
64+
assert log_gaussian.log_prior_from_value(log_gaussian.lower_limit) == -np.inf
65+
66+
67+
def test__other_prior_families_keep_their_limits():
68+
"""
69+
``Prior.limits`` now derives from ``lower_limit``/``upper_limit`` rather than
70+
returning a hardcoded (-inf, inf). GaussianPrior must still be unbounded.
71+
"""
72+
assert af.GaussianPrior(mean=0.0, sigma=1.0).limits == (-np.inf, np.inf)
73+
assert af.UniformPrior(lower_limit=0.0, upper_limit=2.0).limits == (0.0, 2.0)
74+
assert af.LogUniformPrior(lower_limit=0.01, upper_limit=100.0).limits == (
75+
0.01,
76+
100.0,
77+
)
78+
79+
for cls in (af.GaussianPrior, af.UniformPrior, af.LogUniformPrior):
80+
assert cls.lower_limit_strict is False
81+
assert cls.upper_limit_strict is False
82+
83+
84+
@pytest.mark.parametrize(
85+
"value, expected",
86+
[
87+
(-3.0, -np.inf),
88+
(-1e-09, -np.inf),
89+
(0.0, -np.inf),
90+
(1e-12, -204.83588563011642),
91+
(0.001, -8.892033838618836),
92+
(0.1, 0.14164835190717184),
93+
(0.5, 0.3396055360729164),
94+
(1.0, -0.04733727810650888),
95+
(2.0, -0.7185718164978876),
96+
(10.0, -3.3735407249713125),
97+
(1000.0, -19.437601069254285),
98+
],
99+
)
100+
def test__log_prior_from_value_is_unchanged(value, expected):
101+
"""
102+
The density was always correct; only the reported limits were wrong. Pinned
103+
against the pre-change values either side of zero.
104+
"""
105+
prior = af.LogGaussianPrior(mean=0.4, sigma=1.3)
106+
assert prior.log_prior_from_value(value) == pytest.approx(expected, rel=1e-12)
107+
108+
109+
@pytest.mark.parametrize(
110+
"unit, expected",
111+
[
112+
(1e-09, 0.0006129978595719644),
113+
(0.0001, 0.011858368820420245),
114+
(0.01, 0.07249394511581292),
115+
(0.1, 0.2819523947584061),
116+
(0.25, 0.6207439038545902),
117+
(0.5, 1.4918246976412703),
118+
(0.75, 3.5852803622761034),
119+
(0.9, 7.893321602745905),
120+
(0.99, 30.69968015862632),
121+
(0.999999999, 3630.585196715403),
122+
],
123+
)
124+
def test__unit_cube_mapping_is_unchanged(unit, expected):
125+
"""
126+
The nested samplers work in unit-cube coordinates and map through the prior. The
127+
limits live on the prior while the mapping lives on the message stack, which this
128+
change does not touch -- so no stored nested-sampling result shifts.
129+
"""
130+
prior = af.LogGaussianPrior(mean=0.4, sigma=1.3)
131+
assert prior.value_for(unit) == pytest.approx(expected, rel=1e-12)
132+
assert prior.unit_value_for(prior.value_for(unit)) == pytest.approx(unit, rel=1e-6)
133+
134+
135+
def test__identifier_is_unchanged():
136+
"""
137+
If the declared limits fed the identifier, every existing output directory would
138+
re-key and its stored results would be orphaned. ``__identifier_fields__`` is
139+
("mean", "sigma"), and the limits are instance/class attributes outside it, so the
140+
hash is untouched. Pinned to the pre-change value.
141+
"""
142+
prior = af.LogGaussianPrior(mean=0.4, sigma=1.3)
143+
assert str(Identifier(prior)) == "34cb61ade6bafa6050229e8b6b390235"
144+
145+
146+
def test__declared_support_survives_copy_pickle_and_projection(log_gaussian):
147+
"""
148+
The limits are derived in ``__init__`` rather than carried as parameters, which is
149+
what makes them survive every path that rebuilds the prior -- including the JAX
150+
pytree round-trip, where only (mean, sigma, id) are flattened.
151+
"""
152+
assert copy(log_gaussian).lower_limit == 0.0
153+
assert pickle.loads(pickle.dumps(log_gaussian)).lower_limit == 0.0
154+
155+
samples = np.exp(np.random.default_rng(0).normal(1.0, 2.0, 500))
156+
projected = log_gaussian.project(samples, np.zeros(500))
157+
assert projected.lower_limit == 0.0
158+
159+
rebuilt = af.LogGaussianPrior.tree_unflatten((), log_gaussian.tree_flatten()[0])
160+
assert rebuilt.lower_limit == 0.0

test_autofit/mapper/prior/test_prior_properties.py

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -271,3 +271,83 @@ def test__from_mode_matches_mean_and_variance(cls, mean, variance):
271271
message = cls.from_mode(np.asarray(mean), np.asarray(variance))
272272
assert float(message.mean) == pytest.approx(mean, rel=1e-6)
273273
assert float(message.variance) == pytest.approx(variance, rel=1e-6)
274+
275+
276+
# === P6: the reported support matches the actual support ===
277+
278+
279+
def interior_probes(prior):
280+
"""
281+
Points that lie strictly inside the prior's *reported* ``limits``.
282+
283+
Every one of them must have finite log prior. If a prior reports a box wider
284+
than its true support, some point in here falls in the gap and the density
285+
there is -inf -- which is precisely the shape of PyAutoFit#1526, where
286+
``LogGaussianPrior`` reported (-inf, inf) for a support of (0, inf).
287+
"""
288+
lo, hi = prior.limits
289+
probes = []
290+
291+
if np.isfinite(lo) and np.isfinite(hi):
292+
width = hi - lo
293+
probes += [lo + 0.25 * width, lo + 0.5 * width, lo + 0.75 * width]
294+
probes += [lo + 1e-9 * width, hi - 1e-9 * width]
295+
elif np.isfinite(lo):
296+
probes += [lo + 1e-9, lo + 1.0, lo + 1e3]
297+
elif np.isfinite(hi):
298+
probes += [hi - 1e-9, hi - 1.0, hi - 1e3]
299+
else:
300+
probes += [-1e6, -1.0, 0.0, 1.0, 1e3]
301+
302+
return probes
303+
304+
305+
@pytest.mark.parametrize("prior", all_priors(), ids=prior_id)
306+
def test__log_prior_is_finite_everywhere_inside_the_reported_limits(prior):
307+
"""
308+
The general form of PyAutoFit#1526. A prior that reports a bound it does not
309+
actually have hands every consumer a licence to evaluate where the density is
310+
zero -- and for a strictly positive parameter, ``log(0)`` or a division by it
311+
is the failure that follows.
312+
"""
313+
for value in interior_probes(prior):
314+
assert np.isfinite(
315+
prior.log_prior_from_value(value)
316+
), f"{prior} reports limits {prior.limits} but log_prior({value}) is not finite"
317+
318+
319+
@pytest.mark.parametrize("prior", all_priors(), ids=prior_id)
320+
def test__log_prior_is_minus_inf_outside_the_reported_limits(prior):
321+
"""
322+
The converse: the reported box must not be *narrower* than the support either.
323+
"""
324+
lo, hi = prior.limits
325+
326+
if np.isfinite(lo):
327+
assert prior.log_prior_from_value(lo - 1.0) == -np.inf
328+
if np.isfinite(hi):
329+
assert prior.log_prior_from_value(hi + 1.0) == -np.inf
330+
331+
332+
@pytest.mark.parametrize("prior", all_priors(), ids=prior_id)
333+
def test__strictness_flags_agree_with_the_density_at_the_bounds(prior):
334+
"""
335+
A bound flagged strict must have zero density on it; a bound not flagged strict
336+
must have finite density on it. This is what lets a consumer clip onto a bound
337+
without a per-type switch -- ``ClipperPriorBox`` insets only the strict ones.
338+
"""
339+
lo, hi = prior.limits
340+
341+
if np.isfinite(lo):
342+
at_lower = prior.log_prior_from_value(lo)
343+
if prior.lower_limit_strict:
344+
assert at_lower == -np.inf
345+
else:
346+
assert np.isfinite(at_lower)
347+
348+
if np.isfinite(hi):
349+
at_upper = prior.log_prior_from_value(hi)
350+
if prior.upper_limit_strict:
351+
assert at_upper == -np.inf
352+
else:
353+
assert np.isfinite(at_upper)

test_autofit/non_linear/test_clipper.py

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -69,16 +69,24 @@ def test__gaussian_prior__passes_through_as_infinite(self):
6969
assert not np.isnan(lower).any()
7070
assert not np.isnan(upper).any()
7171

72-
def test__log_gaussian_prior__lower_bound_is_declared_by_the_clipper(self):
72+
def test__log_gaussian_prior__lower_bound_is_declared_by_the_prior(self):
7373
"""
74-
LogGaussianPrior reports (-inf, inf) because its TransformedMessage is never
75-
given limits, yet ``log_prior_from_value`` is -inf for value <= 0. The
76-
clipper declares the real (0, inf) support, strictly, so the projected value
74+
LogGaussianPrior declares its own (0, inf) support and flags the lower bound
75+
strict, so the clipper reads it like any other prior and the projected value
7776
lands *above* zero rather than on it.
77+
78+
This test used to assert ``prior.lower_limit == -np.inf`` and the clipper
79+
supplied the real bound itself via an ``isinstance`` switch. That was a
80+
workaround for PyAutoFit#1526: the prior was telling every consumer, not just
81+
this one, that a strictly positive parameter could go negative. The bounds
82+
assertions below are unchanged by the fix — that equivalence is the point.
7883
"""
7984
prior = af.LogGaussianPrior(mean=0.0, sigma=1.0)
8085

81-
assert prior.lower_limit == -np.inf
86+
assert prior.lower_limit == 0.0
87+
assert prior.upper_limit == np.inf
88+
assert prior.lower_limit_strict is True
89+
assert prior.upper_limit_strict is False
8290

8391
model = _model(alpha=prior)
8492
lower, upper = ClipperPriorBox(strict_epsilon=1.0e-12).bounds_from_model(

0 commit comments

Comments
 (0)