From d748fb5af044c1deb066218223f14e309362b4de Mon Sep 17 00:00:00 2001 From: Jammy2211 Date: Thu, 27 Aug 2026 19:02:29 -0400 Subject: [PATCH] fix: Result.instance falls back to a valid sample; write samples before materialising the instance (#1535, #1487) --- autofit/non_linear/result.py | 42 ++++++ autofit/non_linear/search/updater.py | 31 +++-- test_autofit/non_linear/result/test_result.py | 85 ++++++++++++ .../non_linear/search/test_updater.py | 123 ++++++++++++++++++ 4 files changed, 272 insertions(+), 9 deletions(-) diff --git a/autofit/non_linear/result.py b/autofit/non_linear/result.py index 042a954e0..8f2140fdc 100644 --- a/autofit/non_linear/result.py +++ b/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): diff --git a/autofit/non_linear/search/updater.py b/autofit/non_linear/search/updater.py index d4cbae337..560abb8f5 100644 --- a/autofit/non_linear/search/updater.py +++ b/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 diff --git a/test_autofit/non_linear/result/test_result.py b/test_autofit/non_linear/result/test_result.py index 0a25b7584..f74e51dd6 100644 --- a/test_autofit/non_linear/result/test_result.py +++ b/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 diff --git a/test_autofit/non_linear/search/test_updater.py b/test_autofit/non_linear/search/test_updater.py index 79003f1eb..afa15c518 100644 --- a/test_autofit/non_linear/search/test_updater.py +++ b/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()