fix(algorithms): an objective value that cannot be ordered is discarded, not passed on (#713)

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

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