fix: Result.instance falls back to a valid sample; persist samples before materialisation (#1535)

autofit/non_linear/result.py

@@ -114,11 +114,53 @@ def log_likelihood(self):
 
     @property
     def instance(self):
+        """
+        The maximum log likelihood model instance of the fit.
+
+        The instance normally comes from the `samples_summary`, which is cheap to load and is the only
+        result available when a run is resumed with `samples.csv` output disabled.
+
+        A summary stores a *single* sample, so when the current model rejects that stored vector with
+        `FitException` there is nothing for it to substitute and its `to_instance` policy (correctly)
+        raises `SamplesException` -- that policy is not changed here. The full `samples`, when they are
+        available, do carry the other stored points, and `Samples.max_log_likelihood` falls back to the
+        highest-likelihood point the model can still reconstruct (see PyAutoFit #1486). A rejected best
+        point therefore degrades to that recovering path with a warning, instead of killing a completed
+        fit at results-write (PyAutoFit #1535).
+        """
+        recovered = getattr(self, "_recovered_instance", None)
+
+        if recovered is not None:
+            # The recovery scans every stored sample, and `instance` is read many times by
+            # downstream results (e.g. `max_log_likelihood_tracer`), so it is cached to keep
+            # that cost -- and the warning below -- to one occurrence.
+            return recovered
+
         try:
             return self.samples_summary.instance
         except AttributeError as e:
             logging.warning(e)
             return None
+        except (exc.SamplesException, exc.FitException) as e:
+            samples = self.samples
+
+            if samples is None:
+                logging.warning(
+                    f"The maximum log likelihood sample of this result cannot be reconstructed as a "
+                    f"model instance and the full samples are not available to fall back on, so the "
+                    f"instance is None:\n{e}"
+                )
+                return None
+
+            logging.warning(
+                f"The maximum log likelihood sample stored in the samples summary cannot be "
+                f"reconstructed as a model instance, falling back to the highest likelihood sample "
+                f"the model still accepts:\n{e}"
+            )
+
+            self._recovered_instance = samples.max_log_likelihood()
+
+            return self._recovered_instance
 
     @property
     def max_log_likelihood_instance(self):

autofit/non_linear/search/updater.py

@@ -201,26 +201,39 @@ def _save_samples(
         """
         Generate and persist samples.
 
+        Everything that does not need a model instance -- the weight-thresholded
+        `samples.csv` and the `samples_summary.json` -- is written *first*, so a stored
+        best point the current model rejects can never cost a completed fit its data
+        (PyAutoFit #1535, and the reason the weight-threshold prune did not run, #1487).
+        Only the instance-dependent outputs (latents, visualization, profiling) are
+        skipped when materialization fails.
+
         Returns (samples, samples_summary, instance, samples_save).
-        ``instance`` is ``None`` when the fit has failed, signalling the
-        caller to return early.
+        ``instance`` is ``None`` when the stored best point cannot be reconstructed,
+        signalling the caller to skip the instance-dependent outputs.
         """
         samples = self._samples_from(model, search_internal)
         samples_summary = samples.summary()
 
-        try:
-            instance = samples_summary.instance
-        except (exc.FitException, exc.SamplesException):
-            return samples, samples_summary, None, samples
-
-        self._paths.save_samples_summary(samples_summary=samples_summary)
-
         log_message = not during_analysis and not self._disable_output
 
         samples_save = samples.samples_above_weight_threshold_from(
             log_message=log_message
         )
         self._paths.save_samples(samples=samples_save)
+        self._paths.save_samples_summary(samples_summary=samples_summary)
+
+        try:
+            instance = samples_summary.instance
+        except (exc.FitException, exc.SamplesException) as e:
+            logger.warning(
+                "The maximum log likelihood sample cannot be reconstructed as a model "
+                "instance, so the outputs which require one (latent variables, "
+                "visualization, profiling) are skipped for this update. The samples and "
+                "samples summary have still been written to the output folder:\n%s",
+                e,
+            )
+            return samples, samples_summary, None, samples_save
 
         return samples, samples_summary, instance, samples_save
 

test_autofit/non_linear/result/test_result.py

@@ -119,3 +119,88 @@ def test_with_index(self, results):
     def test_missing_result(self, results):
         with pytest.raises(af.exc.PipelineException):
             results.from_name("third")
+
+
+class _RejectsLowValue:
+    """
+    A model component whose constructor rejects part of its own prior range, mimicking a
+    validated parameterization (e.g. `ell_comps` outside the ellipticity unit disk) which
+    only fires when a stored vector is materialized on the host.
+    """
+
+    def __init__(self, value):
+        if value < 0.75:
+            raise af.exc.FitException("value must be at least 0.75")
+        self.value = value
+
+
+class _NoSamplesPaths:
+    """Paths double standing in for a resumed run whose `samples.csv` was never written."""
+
+    @property
+    def samples(self):
+        raise FileNotFoundError()
+
+
+def _samples_with_rejected_best():
+    """
+    Samples whose maximum likelihood point is rejected by the model, but whose
+    second-best point is not.
+    """
+    model = af.Model(_RejectsLowValue)
+    model.value = af.UniformPrior(lower_limit=0.0, upper_limit=1.0)
+
+    return af.SamplesPDF(
+        model=model,
+        sample_list=af.Sample.from_lists(
+            model=model,
+            parameter_lists=[[0.1], [0.9]],
+            log_likelihood_list=[2.0, 1.0],
+            log_prior_list=[0.0, 0.0],
+            weight_list=[1.0, 1.0],
+        ),
+        samples_info={"log_evidence": 1.0},
+    )
+
+
+class TestResultInstanceRejectedBestPoint:
+    def test__summary_still_raises_on_a_rejected_best_point(self):
+        """
+        The summary holds a single sample, so it has nothing to substitute and must keep
+        raising -- the recovery belongs on the full samples, not here.
+        """
+        samples = _samples_with_rejected_best()
+
+        with pytest.raises(af.exc.SamplesException):
+            samples.summary().instance
+
+    def test__result_instance__falls_back_to_the_best_valid_sample(self):
+        samples = _samples_with_rejected_best()
+
+        result = af.Result(
+            samples_summary=samples.summary(),
+            samples=samples,
+        )
+
+        assert result.instance.value == pytest.approx(0.9)
+        assert result.max_log_likelihood_instance.value == pytest.approx(0.9)
+
+    def test__result_instance__is_none_when_there_are_no_samples_to_recover_from(self):
+        samples = _samples_with_rejected_best()
+
+        result = af.Result(
+            samples_summary=samples.summary(),
+            paths=_NoSamplesPaths(),
+        )
+
+        assert result.instance is None
+
+    def test__result_instance__recovered_instance_is_computed_once(self):
+        samples = _samples_with_rejected_best()
+
+        result = af.Result(
+            samples_summary=samples.summary(),
+            samples=samples,
+        )
+
+        assert result.instance is result.instance

test_autofit/non_linear/search/test_updater.py

@@ -2,7 +2,9 @@
 from unittest.mock import MagicMock
 
 from autonerves import conf
+from autonerves.conf import with_config
 
+import autofit as af
 from autofit.non_linear.search.updater import SearchUpdater
 
 
@@ -80,3 +82,124 @@ def test__compute_latent_samples__config_gate_still_works(monkeypatch):
     finally:
         conf.instance["output"]["latent_after_fit"] = original_after
         conf.instance["output"]["latent_during_fit"] = original_during
+
+
+class _RejectsLowValue:
+    """
+    A model component whose constructor rejects part of its own prior range, mimicking a
+    validated parameterization (e.g. `ell_comps` outside the ellipticity unit disk) which
+    only fires when a stored vector is materialized on the host.
+    """
+
+    def __init__(self, value):
+        if value < 0.75:
+            raise af.exc.FitException("value must be at least 0.75")
+        self.value = value
+
+
+def _samples_with_rejected_best():
+    """
+    Samples whose maximum likelihood point is rejected by the model, with a second
+    zero-weight row so the weight-threshold prune has something to remove.
+    """
+    model = af.Model(_RejectsLowValue)
+    model.value = af.UniformPrior(lower_limit=0.0, upper_limit=1.0)
+
+    return af.SamplesPDF(
+        model=model,
+        sample_list=af.Sample.from_lists(
+            model=model,
+            parameter_lists=[[0.1], [0.9]],
+            log_likelihood_list=[2.0, 1.0],
+            log_prior_list=[0.0, 0.0],
+            weight_list=[1.0, 0.0],
+        ),
+        samples_info={"log_evidence": 1.0},
+    )
+
+
+def _updater_for(samples, paths) -> SearchUpdater:
+    return SearchUpdater(
+        paths=paths,
+        timer=MagicMock(),
+        search_logger=logging.getLogger("test_updater"),
+        plot_results_func=MagicMock(),
+        samples_from_func=lambda model, search_internal: samples,
+        disable_output=False,
+        iterations_per_full_update=1.0,
+    )
+
+
+def test__save_samples__persists_samples_when_the_best_point_is_rejected():
+    """
+    A stored best point the model rejects must not cost the run its samples: both
+    `save_samples` and `save_samples_summary` are called, and only the instance is
+    withheld (PyAutoFit #1535).
+    """
+    samples = _samples_with_rejected_best()
+    paths = MagicMock()
+
+    (
+        samples_out,
+        samples_summary,
+        instance,
+        samples_save,
+    ) = _updater_for(samples, paths)._save_samples(
+        model=samples.model,
+        search_internal=None,
+        during_analysis=False,
+    )
+
+    assert instance is None
+    assert samples_out is samples
+    assert samples_summary is not None
+
+    paths.save_samples.assert_called_once()
+    paths.save_samples_summary.assert_called_once()
+
+    assert paths.save_samples.call_args.kwargs["samples"] is samples_save
+
+
+def test__save_samples__weight_threshold_prune_runs_when_the_best_point_is_rejected():
+    """
+    The early return this replaces also skipped the weight-threshold prune, so every
+    zero-weight row survived into `samples.csv` (PyAutoFit #1487).
+    """
+    samples = _samples_with_rejected_best()
+    paths = MagicMock()
+
+    _, _, instance, samples_save = _updater_for(samples, paths)._save_samples(
+        model=samples.model,
+        search_internal=None,
+        during_analysis=False,
+    )
+
+    assert instance is None
+    assert len(samples.sample_list) == 2
+    assert [sample.weight for sample in samples_save.sample_list] == [1.0]
+
+
+@with_config(
+    "general",
+    "output",
+    "samples_to_csv",
+    value=True,
+)
+def test__save_samples__writes_samples_csv_when_the_best_point_is_rejected(
+    output_directory,
+):
+    samples = _samples_with_rejected_best()
+    paths = af.DirectoryPaths(
+        "rejected_best_point",
+        path_prefix=str(output_directory),
+    )
+
+    _, _, instance, _ = _updater_for(samples, paths)._save_samples(
+        model=samples.model,
+        search_internal=None,
+        during_analysis=False,
+    )
+
+    assert instance is None
+    assert (paths._files_path / "samples.csv").exists()
+    assert (paths._files_path / "samples_summary.json").exists()