diff --git a/docs/fit_flags.rst b/docs/fit_flags.rst index 7f1569e4..e433fa8b 100644 --- a/docs/fit_flags.rst +++ b/docs/fit_flags.rst @@ -1,9 +1,9 @@ Orbit fit status ======================================================================================== -Every fit that ``layup orbitfit`` produces reports its outcome in six columns: a -single-value summary, ``flag``, and five columns that report the individual facts -behind it. +Every fit that ``layup orbitfit`` produces reports its outcome in seven columns: a +single-value summary, ``flag``, a plain ``accepted`` predicate, and five columns +that report the individual facts behind them. .. list-table:: :header-rows: 1 @@ -11,6 +11,10 @@ behind it. * - Column - Meaning + * - ``accepted`` + - ``1`` if the fit is usable: it converged and passed every check. The same + thing ``flag == 0`` says, as a predicate you can filter on without knowing + the flag values. * - ``flag`` - Summary. ``0`` if and only if the fit converged and passed every check. * - ``converged`` @@ -23,7 +27,11 @@ behind it. the acceptance threshold. * - ``failed_cov`` - ``1`` if the fit converged but its covariance is degenerate, or a variance is - non-positive. + non-positive. This check runs **only when non-gravitational parameters are + being fitted**, where it detects a non-gravitational term that has become + collinear with the state. A gravity-only fit is never marked here, so a + column of zeros does not mean the covariances were examined and found + sound. * - ``failed_physical`` - ``1`` if the fit converged but describes an orbit no real object could occupy: its hyperbolic excess speed is implausibly large. @@ -35,9 +43,9 @@ matches ``flag == 0``. Which column should I filter on? ---------------------------------------------------------------------------------------- -**For orbits you intend to use, filter on** ``flag == 0``. That is the summary, it -means the fit converged and every check passed, and it is what the rest of ``layup`` -uses internally. +**For orbits you intend to use, filter on** ``accepted == 1``, or equivalently +``flag == 0``. Both mean the fit converged and every check passed, and ``flag == 0`` +is what the rest of ``layup`` uses internally. Read the other columns when you need to know *why* something was rejected — for triage, for diagnosing a survey's failure modes, or to accept fits that failed a diff --git a/src/layup/constants.py b/src/layup/constants.py index 2bba6f40..c1edb76a 100644 --- a/src/layup/constants.py +++ b/src/layup/constants.py @@ -100,7 +100,13 @@ # is zero across all of them, matching ``flag == FLAG_CONVERGED``. A ``passed_*`` # convention would make a never-attempted fit (all zero) indistinguishable from # one that failed everything. -OUTCOME_COLUMNS = ("converged", "stage", "failed_csq", "failed_cov", "failed_physical") +# +# ``accepted`` is the exception, and the one to filter on: it is the summary +# verdict as a predicate, 1 exactly when ``flag == FLAG_CONVERGED``. It reads +# without knowing the flag taxonomy, and unlike an ``error``/``success`` rename +# it does not label -1 (never attempted) or 8 (incremental bookkeeping) as +# errors, which they are not (issue #498). +OUTCOME_COLUMNS = ("accepted", "converged", "stage", "failed_csq", "failed_cov", "failed_physical") # Which check each of the fitter's own post-convergence verdicts reports as # failed. Both are set *after* the Levenberg-Marquardt loop converges, so each diff --git a/src/layup/orbitfit.py b/src/layup/orbitfit.py index 04665942..5f99731b 100644 --- a/src/layup/orbitfit.py +++ b/src/layup/orbitfit.py @@ -742,9 +742,16 @@ def record(self, fit): if gate is not None: setattr(self, gate, True) - def as_row(self): - """The output columns, in ``OUTCOME_COLUMNS`` order.""" + def as_row(self, flag): + """The output columns, in ``OUTCOME_COLUMNS`` order. + + ``accepted`` is derived from the flag the row actually carries rather + than tracked alongside these facts, so the two cannot disagree. It is + taken last, after any stage marker has overwritten the fitter's own + verdict, because that overwritten value is what a reader will see. + """ return ( + int(flag == FLAG_CONVERGED), int(self.converged), int(self.stage), int(self.failed_csq), @@ -807,7 +814,7 @@ def create_empty_result(id, dtypes): ) + (np.nan,) * 36 # Flat covariance matrix # never attempted: not converged, no stage reached, no gate applied - + ((0, STAGE_NOT_ATTEMPTED, 0, 0, 0) if "converged" in dtypes.names else ()) + + (FitOutcome().as_row(FLAG_NOT_ATTEMPTED) if "converged" in dtypes.names else ()) # non-grav columns (issue #351): NaN per a1/a2/a3 (+ _unc) that is present + tuple(np.nan for n in ("a1", "a2", "a3") if n in dtypes.names for _ in (0, 1)) # obs fingerprint (issue #419): empty hash never matches, so a failed @@ -1571,7 +1578,7 @@ def _orbitfit( ("BCART_EQ" if success else "NONE"), # The base format returned by the C++ code ) + cov_matrix # Flat covariance matrix - + outcome.as_row() # the outcome columns + + outcome.as_row(res.flag) # the outcome columns + nongrav_cols # non-grav params + uncertainties (issue #351), when fit_nongrav + per_arc_cols # later-arc amplitudes (comet linkage), when per_arc + (obs_hash, nobs_fit) # obs fingerprint (issue #419) @@ -1951,7 +1958,7 @@ def _fitresult_to_row(fit, obj_id, obs_hash, nobs_fit, dtypes): ) + cov # The sequential update does not run the staged pipeline either. - + (FitOutcome.from_flag(fit.flag).as_row() if "converged" in dtypes.names else ()) + + (FitOutcome.from_flag(fit.flag).as_row(fit.flag) if "converged" in dtypes.names else ()) + (obs_hash, nobs_fit) ) return np.array([row], dtype=dtypes) diff --git a/tests/layup/test_accepted_column.py b/tests/layup/test_accepted_column.py new file mode 100644 index 00000000..d5ebb81c --- /dev/null +++ b/tests/layup/test_accepted_column.py @@ -0,0 +1,123 @@ +"""One column that says whether a fit is usable, without reading the taxonomy. + +Reviewers on #495 asked for a status column that reads as a predicate -- +``if (!error)`` or ``if (success)`` -- rather than an integer to be compared +against a documented list. The two are not interchangeable: ``0`` means +converged today, so ``!error`` works with the values unchanged while ``success`` +is false for every good fit unless the values also invert. And renaming ``flag`` +is breaking: it appears across three modules, sixteen test files and seven +fixture CSVs that carry it as a column header, so output already written stops +loading. + +``accepted`` is the alternative from #498: keep ``flag``, add a derived boolean. +It reads the same, inverts nothing, and does not label ``-1`` (never attempted) +or ``8`` (incremental bookkeeping) as errors, which they are not. + +It is derived from the flag the row actually carries, not tracked beside the +other outcome facts, so the two cannot disagree -- and it is read *after* any +stage marker has overwritten the fitter's own verdict, because the overwritten +value is what a reader sees. +""" + +import numpy as np +import pytest + +import layup.orbitfit as orbitfit +from layup.constants import ( + FLAG_BUILDUP_FAILED, + FLAG_CONVERGED, + FLAG_CSQ_TOO_LARGE, + FLAG_DEGENERATE_COV, + FLAG_DID_NOT_CONVERGE, + FLAG_IMPLAUSIBLE_ORBIT, + FLAG_INCREMENTAL_NO_FULL_OBS, + FLAG_NO_ROOT_CONVERGED, + FLAG_NO_SOLUTION, + FLAG_NOT_ATTEMPTED, + OUTCOME_COLUMNS, + STAGE_COMPLETE, +) +from layup.orbitfit import FitOutcome + +ACCEPTED = OUTCOME_COLUMNS.index("accepted") + +# Every documented flag other than the accepting one. +REJECTING_FLAGS = [ + FLAG_NOT_ATTEMPTED, + FLAG_DID_NOT_CONVERGE, + FLAG_CSQ_TOO_LARGE, + FLAG_NO_ROOT_CONVERGED, + FLAG_BUILDUP_FAILED, + FLAG_NO_SOLUTION, + FLAG_DEGENERATE_COV, + FLAG_INCREMENTAL_NO_FULL_OBS, + FLAG_IMPLAUSIBLE_ORBIT, +] + + +def test_accepted_is_the_first_outcome_column(): + """It is the one to filter on, so it leads the group rather than sitting + among the individual verdicts.""" + assert OUTCOME_COLUMNS[0] == "accepted" + + +def test_accepted_is_one_exactly_for_the_converged_flag(): + row = FitOutcome(converged=True, stage=STAGE_COMPLETE).as_row(FLAG_CONVERGED) + assert row[ACCEPTED] == 1 + + +@pytest.mark.parametrize("flag", REJECTING_FLAGS) +def test_every_other_flag_is_not_accepted(flag): + """A single value means usable and it is the only one. Anything else -- a + rejection, a stage marker, or the never-attempted sentinel -- is not.""" + row = FitOutcome.from_flag(flag).as_row(flag) + assert row[ACCEPTED] == 0, f"flag {flag} should not read as accepted" + + +def test_accepted_follows_the_flag_the_row_carries_not_the_fitters_verdict(): + """The case #499 is about: a fit converged, was rejected on chi-square, and + then had its flag overwritten by a stage marker. The row carries 4, so it is + not accepted -- even though the outcome facts still record that it converged. + Deriving from the flag is what keeps the two consistent.""" + outcome = FitOutcome(converged=True, stage=STAGE_COMPLETE, failed_csq=True) + row = outcome.as_row(FLAG_BUILDUP_FAILED) + assert row[ACCEPTED] == 0 + assert row[OUTCOME_COLUMNS.index("converged")] == 1 + assert row[OUTCOME_COLUMNS.index("failed_csq")] == 1 + + +def test_accepted_is_a_plain_int_like_the_other_columns(): + """The row goes straight into an i1 column; a bool would be stored without + complaint.""" + row = FitOutcome(converged=True).as_row(FLAG_CONVERGED) + assert all(isinstance(v, int) for v in row) + assert len(row) == len(OUTCOME_COLUMNS) + + +def test_never_attempted_is_not_accepted_and_not_a_failure(): + """The reason #498 preferred this over renaming flag to ``error``: a row that + was never fit is not an error, it is an absence. It reads as not accepted, + with no check marked failed.""" + row = orbitfit.create_empty_result("x", orbitfit._get_result_dtypes("ObjID", [])) + assert row["accepted"][0] == 0 + assert row["flag"][0] == FLAG_NOT_ATTEMPTED + assert row["failed_csq"][0] == 0 + assert row["failed_cov"][0] == 0 + assert row["failed_physical"][0] == 0 + + +def test_accepted_agrees_with_the_flag_across_the_whole_taxonomy(): + """The column exists so that callers stop writing ``flag == 0`` by hand. If + the two ever disagree, the column is worse than nothing.""" + for flag in [FLAG_CONVERGED] + REJECTING_FLAGS: + row = FitOutcome.from_flag(flag).as_row(flag) + assert row[ACCEPTED] == int(flag == FLAG_CONVERGED), f"disagreement at flag {flag}" + + +def test_the_column_is_in_the_output_dtype(): + dt = orbitfit._get_result_dtypes("ObjID", []) + assert "accepted" in dt.names + assert np.dtype(dt["accepted"]).kind == "i" + # It stays inside the outcome group, ahead of the fingerprint columns. + assert dt.names.index("flag") < dt.names.index("accepted") + assert dt.names[-2:] == ("obs_hash", "nobs_fit") diff --git a/tests/layup/test_fit_outcome.py b/tests/layup/test_fit_outcome.py index 9dc0bf8c..680d6bbc 100644 --- a/tests/layup/test_fit_outcome.py +++ b/tests/layup/test_fit_outcome.py @@ -176,10 +176,11 @@ def test_as_row_is_ints_in_column_order(): plain ints in OUTCOME_COLUMNS order. A bool or a reordering would be stored without complaint and silently mislabel every fit.""" outcome = FitOutcome(converged=True, stage=STAGE_COMPLETE, failed_csq=True) - row = outcome.as_row() + row = outcome.as_row(FLAG_CSQ_TOO_LARGE) assert len(row) == len(OUTCOME_COLUMNS) assert all(isinstance(v, int) for v in row) - assert row == (1, STAGE_COMPLETE, 1, 0, 0) + # converged, then rejected on chi-square: accepted is 0 though converged is 1. + assert row == (0, 1, STAGE_COMPLETE, 1, 0, 0) # --------------------------------------------------------------------------