Report a converged but physically impossible orbit as flag 9 (#493) by matthewholman · Pull Request #497 · Smithsonian/layup

matthewholman · 2026-08-27T18:06:33Z

Adds flag 9: converged, but the orbit is not physically possible.

The criterion is hyperbolic excess speed, not boundedness. Layup is expected to
fit interstellar objects, and those are unbound and fast -- 3I/ATLAS arrives at
about 59 km/s -- so an unbound orbit is never on its own grounds for rejection.
Default threshold 200 km/s (orbitfit.MAX_EXCESS_SPEED_KM_S, 0 disables it).

Measured on 196 short arcs cut from MPC histories, with a long-arc fit of the
same object as truth: the gate rejected no correct fit, and every fit it
rejected was wrong. It is a floor, not a filter -- it catches the unambiguous
cases and cannot reach fits that are plausible and still wrong.

Applied to the result the caller receives, not inside the C++ fit: the driver
runs one fit per IOD candidate and reads a non-zero flag as "candidate failed",
so a per-candidate verdict is retried away and reported as flag 3.

Like flag 2, a gated fit reports state and covariance as NaN.

Two things found on the way, both filed separately rather than fixed here: 59%
of flag=0 short-arc fits have the wrong semimajor axis (#485 says 37%, measured
against a physicality proxy rather than truth), and orbitfit.py overwrites the
C++ flag on two paths.

Toward #493.

A short arc can converge with an excellent reduced chi-square onto a state no real object could occupy, because the arc does not constrain the velocity. The chi-square gate cannot catch this: it is anti-correlated with the failure, since the less the object moves across the arc the better the fit (#485). Measured against long-arc truth orbits, 59% of flag=0 short-arc fits have the wrong semimajor axis. The criterion is hyperbolic excess speed, not boundedness. Layup is expected to fit genuine interstellar objects, and those are unbound and fast -- 3I/ATLAS arrives at about 59 km/s -- so an unbound orbit is never on its own grounds for rejection. The default threshold is 200 km/s, about 3.4x the fastest interstellar object yet observed; on 196 measured short-arc fits it rejected no correct fit, and every fit it rejected was wrong. Applied to the result the caller receives rather than inside the C++ fit. The driver runs one fit per IOD candidate and reads a non-zero flag as 'this candidate failed', so a verdict set per-candidate is retried away and reported as flag 3 or 4.

mschwamb · 2026-08-31T13:23:27Z

            else:
                logger.debug("Non-grav refinement did not converge; reporting non-grav params as NaN.")

+        # Physical-plausibility gate (issue #493). Applied here, to the result the


Let's not put issues into the doc strings This does not help maintainable.

mschwamb · 2026-08-31T13:23:53Z

+A short arc can converge with an excellent reduced chi-square onto a state no
+real object could occupy, because the arc does not constrain the velocity. The
+chi-square gate cannot catch this -- it is anti-correlated with the failure,
+since the less the object moves the better the fit (issue #485).


no issues in doc strings.

mschwamb · 2026-08-31T13:28:18Z

+# speed is evidence of a bad fit rather than an unusual object. The default is
+# about 3.4x the fastest interstellar object yet observed. Measured against
+# long-arc truth orbits it rejected no correct fit, and every fit it rejected was
+# wrong. Set to 0 to disable the check.


Do we need all this text as a doc string/comment that duplicates what is in function. I think a shorter summary here would be useful.

mschwamb · 2026-08-31T13:32:07Z

+import layup.orbitfit as orbitfit_module
+from layup.orbitfit import FLAG_IMPLAUSIBLE_ORBIT
+
+GM_SUN = 2.9591220828559115e-4  # au^3/day^2


why are we not using constants.py to store these values?

mschwamb · 2026-08-31T13:32:12Z

+from layup.orbitfit import FLAG_IMPLAUSIBLE_ORBIT
+
+GM_SUN = 2.9591220828559115e-4  # au^3/day^2
+KM_S_IN_AU_DAY = 86400.0 / 149597870.7


why are we not using constants.py to store these values?

mschwamb · 2026-08-31T13:34:56Z

+# interesting side of the gate, and layup already ships its discovery-arc
+# astrometry. Moving the threshold across its excess speed must move the flag,
+# which tests the gate itself rather than the accessor.
+# ---------------------------------------------------------------------------


A natural fixture? It doesn't matter whether layup ships the observations in a demo or something else. This is not relevant to a code comment about what the code is doing. Tests should be appropriate whether or not the data is available already or not. I'm not sure which is the fastest. I'd either remove or revise this entire comment.

mschwamb · 2026-08-31T13:36:21Z

+# wrong. Set to 0 to disable the check.
+MAX_EXCESS_SPEED_KM_S = 200.0
+
+_KM_S_IN_AU_PER_DAY = 86400.0 / 149597870.7


why are we not using constants.py to store these constants in?

mschwamb · 2026-08-31T13:42:34Z

+
+
+def set_max_v_inf(km_s):
+    orbitfit_module.MAX_EXCESS_SPEED_KM_S = km_s


why are we changing the values of constants? I get wary when tests does this. They should be able to set with what's already in the constant set in the code. A unit test can simply be written without having to edit all of this?

mschwamb · 2026-08-31T13:47:07Z

+    than heliocentric, and the Sun's mass rather than the whole solar system's,
+    are both far below the threshold that matters here.
+    """
+    if not MAX_EXCESS_SPEED_KM_S > 0:


I think this is really bad to have 0 be turning off this check - just setting the implausible speed to very high would do the same thing keep the same logic or set some kind of flag to turn this check off

mschwamb · 2026-08-31T13:48:29Z

+    ``state`` is the barycentric equatorial state (au, au/day). Barycentric rather
+    than heliocentric, and the Sun's mass rather than the whole solar system's,
+    are both far below the threshold that matters here.
+    """


doc stings re not in the same format as elsewhere in layup

mschwamb · 2026-08-31T13:49:47Z

+def test_default_threshold_clears_the_fastest_known_interstellar_object():
+    """The default must not reject a real interstellar object.
+
+    Layup's stated advantage over OrbFit and OpenOrb is that it handles the


Can we not comment about other people's packages in comments?

mschwamb · 2026-08-31T13:51:48Z

+# occupy (issue #493). It sits with flag 2 (chi-square too large) and flag 6
+# (degenerate covariance) as a fit that converged and was then rejected by a
+# gate, rather than with the flags for fits that never converged.
+FLAG_IMPLAUSIBLE_ORBIT = 9


where are the other flags defined. I think all flags should be easily findable with their numbers. Maybe we need another file that stores those constants?

mschwamb

Feedback for consideration

Per review feedback on #497 that issue numbers in docstrings do not help maintainability. The explanations stay; only the numbers go. Commit messages remain the durable place for the issue trail.

Resolves against the outcome columns that shipped in #513.

Follows #497, which applies the hyperbolic-excess-speed check and adds FLAG_IMPLAUSIBLE_ORBIT. failed_physical is no longer reserved. Says what the check does and does not do: it catches the extreme case, the threshold is deliberately generous because layup is expected to fit real interstellar objects, and it will therefore accept short-arc orbits that are implausible without being impossible. Names the constant and says which way to move it. DEPENDS ON #497 - flag 9 does not exist until that merges.

matthewholman · 2026-08-31T23:06:58Z

I believe your concerns have been addressed, @mschwamb

Review asked for one place that defines the flags, so a docstring does not have to be maintained alongside them. That place exists -- CONVERGED_FLAGS in constants.py -- but it was incomplete: #497 added FLAG_IMPLAUSIBLE_ORBIT as a converged outcome and never added it here, so the constant contradicted the flag's own comment. That is the maintenance failure the review predicted, already happened once. - CONVERGED_FLAGS gains FLAG_IMPLAUSIBLE_ORBIT, with the rule written down: every flag whose comment begins "converged" belongs in it. - from_flag maps flag 9 to STAGE_COMPLETE and sets failed_physical, so it reconstructs like flags 2 and 6. Before this a flag-9 row came back byte-identical to "did not converge", losing that it had converged and why it was rejected. The contract test covered eight flags and not this one. - the -sf test derives its flags from CONVERGED_FLAGS rather than writing them out, so it tracks the constant. Also per review, the docstrings describe the code rather than the change: the rationale sentence is removed, the flag values are no longer listed in prose, the "had no coverage" framing is dropped, and the mask uses FLAG_CONVERGED rather than a bare 0. 124 tests pass across the outcome, csq, accepted, incremental, sequential, radar and nongrav suites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FWmtkZQeT8EFD4e2eovpm7

This was referenced Aug 27, 2026

Document that flag under-reports gate rejections; triage on the failed_* columns #499

Closed

Unusually long orbitfit execution time for certain MBAs #465

Closed

matthewholman requested review from hannorein and mschwamb August 30, 2026 11:43

mschwamb reviewed Aug 31, 2026

View reviewed changes

mschwamb requested changes Aug 31, 2026

View reviewed changes

matthewholman mentioned this pull request Aug 31, 2026

Report fit outcome as independent columns instead of one flag (#499) #513

Merged

Merge main into feat/493-implausible-orbit-flag

79a1dac

Resolves against the outcome columns that shipped in #513.

mschwamb approved these changes Sep 1, 2026

View reviewed changes

matthewholman merged commit 6e8d197 into main Sep 1, 2026
7 checks passed

matthewholman deleted the feat/493-implausible-orbit-flag branch September 1, 2026 10:07

matthewholman mentioned this pull request Sep 1, 2026

orbitfit reports flag=0 for a 10-minute arc while returning an unbound orbit at 970x circular velocity #485

Closed

matthewholman mentioned this pull request Sep 10, 2026

orbitfit should report what was determined, not just whether it converged #493

Closed



		def set_max_v_inf(km_s):
		orbitfit_module.MAX_EXCESS_SPEED_KM_S = km_s

Uh oh!

Conversation

matthewholman commented Aug 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mschwamb left a comment

Choose a reason for hiding this comment

Uh oh!

matthewholman commented Aug 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants