From 2920b25254885c90b92ed1341a7dd7c7bf0a16c0 Mon Sep 17 00:00:00 2001 From: Bill Hlavacek Date: Thu, 17 Sep 2026 10:33:53 -0600 Subject: [PATCH] fix(algorithms): an objective value that cannot be ordered is discarded, not passed on (#713) Every optimizer decides by comparing scores, so a value that is not an ordinary number does not merely give a wrong answer -- it gives no answer at all, silently. A NaN is inert: nan < best is False so it never becomes the new best, and nan > threshold is False so it is never rejected as bad either. The parameter set is neither accepted nor discarded, and the fit runs to completion reporting nothing. #707 was one producer of such a score; this is why that bug was fatal rather than merely wrong, and fixing it here makes the whole class impossible rather than one instance of it. score_result is the right place because the run loop calls it, through add_to_trajectory, before it reads res.score for the min_objective check and before it hands the result to the algorithm's own got_result -- which is where all fourteen optimizers and samplers read the score. Trajectory.add already mapped NaN to inf, but only for its own heap ordering; res.score itself stayed NaN, and that is the object the optimizers read. Two things were wrong, and the placement was the more serious of them. The guard sat INSIDE the "score it here on the master" branch, so it only ever saw a result this process had just scored. A result the workers had already scored -- the default path, where an ObjectiveCalculator is scattered -- reached no check at all. The guard now runs after both paths converge. Its predicate tested res.score is None. A NaN is not None, so it did not fire. The predicate is now unusable_score, which also rejects -inf (the mirror image of NaN: it wins every comparison forever, so one degenerate evaluation pins itself as the best fit for the rest of the run) and anything not numeric at all, since a user post-processing script assigns res.score directly and is under no obligation to assign a number. +inf is deliberately NOT rejected: it is this codebase's established "discard this parameter set" sentinel, written by the #388 handler and by the worker path, and it orders correctly against every real score. Treating it as unusable would log a spurious warning on every failed simulation. The dispatch above the guard still tests None alone, and must. A NaN is a score that WAS computed, not the absence of one; re-scoring it would re-run normalize() over simdata the workers already normalized -- those transforms rewrite the column in place, so a second pass corrupts it -- on top of repeating the objective. The issue reads the two checks as one kind of guard; they are a dispatch and a validity check, and only the second should widen. The warning now names the offending value instead of asserting the simulation data contained NaNs, which was written for the None case and is not what happened when a finite simulation produces a NaN score (#707's shape exactly). core.py's worker-side guard is also None-only and is left as it is: a NaN there now travels to score_result and is caught at the chokepoint. Fixing it in place would mean moving unusable_score somewhere core.py can import from, since base.py imports core, for no change in behaviour. base.py's confirmation check (score is None or not np.isfinite(score)) keeps its own predicate: there +inf IS a failure of that replicate, which is a different question from whether a score can be ordered. Tests: test_run_loop gains TestUnusableScoreGuard -- the worker-scored path for each unusable value, the master-scored NaN (#707's shape) and None, that a usable score including +inf is returned untouched, that the value is named in the log, that a worker-scored result is not re-normalized or re-scored, and that a NaN never reaches an algorithm's got_result. Seven of the nine fail against the old code; the two that pass are the controls -- the None case it already handled, and the usable scores it must leave alone. Signed-off-by: Bill Hlavacek --- pybnf/algorithms/base.py | 66 +++++++++++++++++++++++---- tests/test_run_loop.py | 99 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 157 insertions(+), 8 deletions(-) diff --git a/pybnf/algorithms/base.py b/pybnf/algorithms/base.py index 6a5ffc6e..1ae57d29 100644 --- a/pybnf/algorithms/base.py +++ b/pybnf/algorithms/base.py @@ -72,6 +72,38 @@ def _bngsim_unavailable_reason(): return BNGSIM_ERROR or 'bngsim is not available' +def unusable_score(score): + """True when an objective value cannot be used to order parameter sets (#713). + + Every optimizer decides by comparing scores, so a value that is not an ordinary + number (or ``+inf``) does not merely give a wrong answer -- it gives no answer at + all, silently: + + * ``None`` -- the objective declined to score (a NaN or Inf *prediction*, the + failed-simulation path). + * ``NaN`` -- **inert in every comparison**. ``nan < best`` is False, so it never + becomes the new best; ``nan > threshold`` is also False, so it is never rejected + as bad either. The parameter set is neither accepted nor discarded, and the fit + finishes reporting no error. lanl/PyBNF#707 was one producer of such a score. + * ``-inf`` -- the mirror image: it wins every comparison forever, so one degenerate + evaluation (a density that overflowed, say) pins itself as the best fit for the + rest of the run. + * anything not numeric at all -- a user post-processing script assigns + ``res.score`` directly and is not obliged to assign a number. + + ``+inf`` is **not** unusable: it is this codebase's established "discard this + parameter set" sentinel, written by the #388 handler and by the worker path, and it + orders correctly against every real score. + """ + if score is None: + return True + try: + value = float(score) + except (TypeError, ValueError): + return True + return np.isnan(value) or value == -np.inf + + class Algorithm(ABC): """Base class for every PyBNF fit type ("method"); defines the run-loop contract. @@ -850,9 +882,19 @@ def score_result(self, res): Split out of :meth:`add_to_trajectory` so the end-of-fit confirmation stage (#659), which scores results it deliberately does not put in the trajectory, goes through exactly the same path the fit did. - """ - # Evaluate objective if it wasn't done on workers. - if res.score is None: # Check if the objective wasn't evaluated on the workers + + This is also where an unusable objective value is turned into ``+inf`` (#713). + Doing it here covers every algorithm: the run loop calls this (through + :meth:`add_to_trajectory`) *before* it reads ``res.score`` for the + ``min_objective`` check and before it hands the result to the algorithm's own + ``got_result``, which is where each optimizer reads the score. + """ + # Evaluate the objective if it wasn't done on workers. This dispatch tests for None + # and nothing else on purpose: a NaN is a score that WAS computed, not the absence of + # one, so re-scoring it here would re-run normalize() over already-normalized simdata + # (the transforms rewrite the column in place) on top of repeating the objective. + # Validity is judged below, after both paths have converged. + if res.score is None: try: res.normalize(self.config.config['normalization']) # Do custom postprocessing, if any @@ -871,11 +913,19 @@ def score_result(self, res): logger.exception(f'Objective evaluation failed for Result {res.name}') res.score = np.inf print1(f'Objective evaluation failed for Result {res.name}; discarding this parameter set') - if res.score is None: # Check if the above evaluation failed - res.score = np.inf - logger.warning(f'Simulation corresponding to Result {res.name} contained NaNs or Infs') - logger.warning(f'Discarding Result {res.name} as having an infinite objective function value') - print1(f'Simulation data in Result {res.name} has NaN or Inf values. Discarding this parameter set') + # Both paths land here -- the master-scored one above AND a result the workers + # already scored, which is the default path and formerly reached no check at all + # (the old guard was nested inside the branch above). An unusable score becomes the + # +inf that means "discard this parameter set", so the optimizers only ever compare + # orderable values. See unusable_score for why NaN and -inf are as bad as None. + if unusable_score(res.score): + unusable = res.score + res.score = np.inf + logger.warning(f'Result {res.name} has an unusable objective value ({unusable!r}), ' + f'which cannot be ordered against other parameter sets') + logger.warning(f'Discarding Result {res.name} as having an infinite objective function value') + print1(f'Result {res.name} has an unusable objective value ({unusable!r}). ' + f'Discarding this parameter set') return res.score def add_to_trajectory(self, res): diff --git a/tests/test_run_loop.py b/tests/test_run_loop.py index 7055742e..37ac7f34 100644 --- a/tests/test_run_loop.py +++ b/tests/test_run_loop.py @@ -543,6 +543,105 @@ def test_cluster_run_reports_parallelism_end_to_end(tmp_path, monkeypatch, caplo assert 'will sit idle' in caplog.text +class TestUnusableScoreGuard: + """``score_result`` turns an objective value that cannot be ordered into ``+inf`` (#713). + + A NaN is the dangerous one because it is **inert**, not wrong: every comparison against + it is False, so ``nan < best`` never promotes it and ``nan > threshold`` never rejects + it. The parameter set is neither accepted nor discarded and the fit ends reporting + nothing. The guard used to test ``is None`` only, and -- more importantly -- it sat + *inside* the "score it here on the master" branch, so a result the workers had already + scored (the default path) reached no check whatsoever. + """ + + # None is deliberately absent here: it does not mean "an unusable score", it means "not + # scored yet", and the branch above the guard exists to score it (see the master-scored + # cases below). The worker path never emits None -- core.py converts it to inf already. + @pytest.mark.parametrize('bad', [float('nan'), -np.inf, 'not a number'], + ids=['nan', 'neg-inf', 'non-numeric']) + def test_a_worker_scored_unusable_value_becomes_inf(self, bad): + """The default path: the workers scored it, so the master does not re-score. This is + the case the old guard could not see at all, being nested in the other branch.""" + algo = _bare_algo() + res = _scored('s1', 1.0) + res.score = bad # what a worker (or a postprocess script) left + assert algo.score_result(res) == np.inf + assert res.score == np.inf + + def test_a_master_scored_none_becomes_inf(self): + """The case the old guard did catch, unchanged: the objective declines to score + (a NaN or Inf prediction) and returns None.""" + algo = _bare_algo() + algo.objective = type('NoneObj', (), { + 'evaluate_multiple': lambda self, *a, **k: None})() + res = _scored('s1', 1.0) + res.score = None + assert algo.score_result(res) == np.inf + + def test_a_master_scored_nan_becomes_inf(self): + """The master-scoring path: score is None on arrival, the objective returns NaN. + This is lanl/PyBNF#707's shape -- ave_norm_sos over a column whose mean was NaN.""" + algo = _bare_algo() + algo.objective = type('NanObj', (), { + 'evaluate_multiple': lambda self, *a, **k: float('nan')})() + res = _scored('s1', 1.0) + res.score = None + assert algo.score_result(res) == np.inf + + def test_a_usable_score_is_returned_untouched(self): + """Including +inf, which is this codebase's own "discard this parameter set" + sentinel (written by the #388 handler and the worker path) and orders correctly -- + so it must not be rewritten, nor warned about on every failed simulation.""" + algo = _bare_algo() + for good in (5.0, 0.0, -5.0, np.inf, 1e300): + res = _scored('s1', 1.0) + res.score = good + assert algo.score_result(res) == good + + def test_the_unusable_value_is_named_in_the_log(self, caplog, capsys): + algo = _bare_algo() + res = _scored('s1', 1.0) + res.score = float('nan') + with caplog.at_level(logging.WARNING, logger='pybnf.algorithms'): + algo.score_result(res) + assert 'unusable objective value' in caplog.text + assert 'nan' in caplog.text.lower() + assert 'unusable objective value' in capsys.readouterr().out + + def test_a_worker_scored_result_is_not_rescored(self, monkeypatch): + """The dispatch above the guard still tests ``is None`` alone. Re-scoring a result + that already has a value would re-run ``normalize`` over simdata the workers already + normalized -- the transforms rewrite the column in place, so a second pass corrupts + it -- on top of repeating the objective. A NaN is a score that was computed, not the + absence of one, so it must be sanitized without being recomputed.""" + algo = _bare_algo() + normalized, scored = [], [] + algo.objective = type('SpyObj', (), { + 'evaluate_multiple': lambda self, *a, **k: scored.append(1) or 1.0})() + res = _scored('s1', 1.0) + res.score = float('nan') + monkeypatch.setattr(type(res), 'normalize', + lambda self, settings: normalized.append(settings), raising=False) + + assert algo.score_result(res) == np.inf + assert normalized == [] and scored == [] # neither was re-run + + def test_nan_never_reaches_the_algorithms_got_result(self): + """The end the guard exists for: an optimizer's ``got_result`` is where every + algorithm reads ``res.score``, and the run loop reaches it only after + ``add_to_trajectory`` -> ``score_result``. So the score an algorithm sees is + orderable even when the objective produced a NaN.""" + seen = [] + algo = _bare_algo(got_result=lambda res: seen.append(res.score) or []) + res = _scored('s1', 1.0) + res.score = float('nan') + + algo._record_result_and_decide(res) + + assert seen == [np.inf] + np.testing.assert_allclose(algo.trajectory.best_score(), np.inf) + + class TestRecordResultAndDecide: def test_success_records_and_returns_next_psets(self):