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