From ee843c2c79d18038e9868b138aa5c027fbf4f4e4 Mon Sep 17 00:00:00 2001 From: matthewholman Date: Wed, 2 Sep 2026 15:23:11 -0400 Subject: [PATCH] Add an accepted column so a usable fit reads as a predicate (#498) Reviewers on #495 asked for a status column that reads as `if (!error)` or `if (success)` rather than an integer 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: seven fixture CSVs carry it as a column header, so output already written stops loading. This is #498's alternative instead: keep `flag`, add a derived boolean. `accepted` is 1 exactly when `flag == FLAG_CONVERGED`. 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 carries rather than tracked beside the other outcome facts, so the two cannot disagree, and it is taken after any stage marker has overwritten the fitter's own verdict -- that overwritten value is what a reader sees. Also replaces the hand-copied outcome tuple in create_empty_result with a real as_row call. It was a second copy of the column layout and would have silently desynced; the existing tests caught it as a length mismatch. Separately, corrects the failed_cov entry in docs/fit_flags.rst. Both of its assignments sit inside the non-gravitational weak-constraint guard, so a gravity-only fit is never marked there -- the page implied the covariance was examined for every fit. --- docs/fit_flags.rst | 22 +++-- src/layup/constants.py | 8 +- src/layup/orbitfit.py | 17 ++-- tests/layup/test_accepted_column.py | 123 ++++++++++++++++++++++++++++ tests/layup/test_fit_outcome.py | 5 +- 5 files changed, 160 insertions(+), 15 deletions(-) create mode 100644 tests/layup/test_accepted_column.py 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) # --------------------------------------------------------------------------