Add universal Bernstein-Khushalani engine (bk_native) to do_fit#326
Merged
Conversation
… Python Without these, accessing rho_hat / a_vec / d_vec from Python raises "Unable to convert function return value to a Python type" because the binding can't see the Eigen and std::array conversions. This is a minimal enabler (no behavior change); the A/D-vector correctness fix on fix/tangent-basis-vectors is independent and lives on its own branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pure C++/Eigen math layer for the Bernstein-Khushalani parameterization,
with the barycenter as the BK coordinate origin and gnomonic projection
defining the tangent plane at a fiducial direction n0. No ASSIST,
REBOUND, or pybind11 dependencies, so this translation unit can be
tested in isolation.
New files in src/lib/orbit_fit/:
bk_basis.h -- types (BKState, BKFiducial) and function declarations
bk_basis.cpp -- implementations: choose_fiducial, bk_to_cartesian,
cartesian_to_bk, dcart_dbk (full 6x6 including the
bottom-left cross-term block), sigma_gdot_sq
The 6x6 Jacobian dcart_dbk has the block structure expected from the
math:
[ d r / d (alpha,beta,gamma) 0 ]
[ d v / d (alpha,beta,gamma) d v / d (adot,bdot,gdot) ]
with the top-left and bottom-right 3x3 blocks identical (both built
from the (1/gamma)-scaled tangent vectors), and the bottom-left block
holding the cross-term contributions through the second derivatives
d^2 rho_hat / d (alpha, beta)^2.
sigma_gdot_sq returns the bound-orbit energy-prior variance,
gamma^2 (2 mu gamma^3 - adot^2 - bdot^2), or +infinity when the
right-hand side is non-positive (tangential rates already exceed
escape). Returning +infinity yields zero precision in the prior
matrix used by the LM step, which is the desired "no prior" behavior.
orbit_fit.cpp adds a single line to its unity-build chain:
#include "bk_basis.cpp"
so the new translation unit compiles into the existing _core module.
No pybind11 binding yet -- that comes in a follow-up commit alongside
Python-side unit tests for the math primitives.
The math derivation, design decisions (barycenter origin, fixed
gdot prior, eigendecomp+energy-prior solver, file layout, layered
test plan) live in the project memory file bk_everywhere_design.md.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a bk_basis_bindings(py::module&) entry point that exposes BKState,
BKFiducial, bk_choose_fiducial, bk_to_cartesian, cartesian_to_bk,
dcart_dbk, and sigma_gdot_sq to Python via pybind11 / pybind11/eigen.h,
and wires the binding into main.cpp's _core module alongside the
existing detection_bindings etc.
tests/layup/test_bk_basis.py covers the pure-math invariants
(25 tests, all passing):
* Round-trip Cartesian <-> BK across mainbelt / NEO / TNO regimes
(rtol 1e-12).
* Analytic dcart_dbk vs central-difference, per-element relative
error < 1e-5 with parameter-scaled epsilon.
* Mixed-partial symmetry of the second-derivative cross-terms
appearing in the bottom-left block of dcart_dbk
(d^2 r / d alpha d beta == d^2 r / d beta d alpha to FD tolerance).
* Fiducial-direction gauge invariance: two valid n0 choices recover
the same Cartesian orbit through round-trip.
* Special-case forms at the fiducial direction alpha = beta = 0:
position is (1/gamma) n0, top-left and bottom-right Jacobian
blocks are [(1/gamma) a, (1/gamma) b, -(1/gamma^2) n0] as columns,
bottom-left block vanishes when rates are zero.
* sigma_gdot_sq agreement with the Cartesian energy bound at the
parabolic boundary, and +inf return when tangential rates already
exceed escape.
The energy-bound test caught a real bug in the first cut of
sigma_gdot_sq: the formula gamma^2 (2 mu gamma^3 - adot^2 - bdot^2)
is only exact at the fiducial direction. Off-fiducial, the gnomonic
tangent vectors rho_hat_alpha, rho_hat_beta have magnitudes
sqrt((1+beta^2))/s^2 and sqrt((1+alpha^2))/s^2 respectively, and an
inner product -alpha*beta/s^4, so the true tangential-velocity term is
|adot rho_hat_alpha + bdot rho_hat_beta|^2 =
[adot^2 (1+beta^2) - 2 adot bdot alpha beta + bdot^2 (1+alpha^2)] / s^4
which reduces to adot^2 + bdot^2 only at alpha = beta = 0. Fixed in
sigma_gdot_sq (and bk_basis.h documentation) to use the exact form,
which reproduces the parabolic-boundary condition |v|^2 = 2 mu / |r|
to machine precision in the test.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bk_fit.cpp contains the LM driver that performs an orbit fit in the
universal Bernstein-Khushalani parameterization on top of layup's
existing Cartesian variational machinery. Included from orbit_fit.cpp
inside the `namespace orbit_fit` block, after the Cartesian helpers
(compute_residuals, create_sequences, get_weight_matrix, converged)
and the Observation/FitResult types are in scope, so no forward
declarations are needed.
The driver structure mirrors run_from_vector_with_initial_guess but
operates in BK basis throughout:
1. Pick a fiducial direction from the observations' rho_hat vectors
(mean direction, Gram-Schmidt for the orthonormal a, b).
2. Convert the Cartesian seed to BK via cartesian_to_bk.
3. Compute a fixed bound-orbit energy prior precision on gdot from
the BK seed (1 / sigma_gdot_sq), zero precision otherwise.
4. LM loop:
- convert current BK state to Cartesian -> reb_particle
- call compute_residuals to get tangent-plane residuals and
Cartesian 6-element partials per observation
- chain-rule: B_bk = B_cart * dcart_dbk(current BK, fiducial)
- assemble C = B_bk^T W B_bk + lambda I + P_prior,
grad = B_bk^T W r + P_prior * p_bk
- solve, Marquardt rho-ratio accept/reject, update BK state,
check convergence (using the existing `converged` predicate).
5. On convergence:
- cov_bk = (B^T W B + P_prior)^-1 (Hessian without lambda)
- cov_cart = J cov_bk J^T (J = dcart_dbk at converged BK state)
- return FitResult with state = bk_to_cartesian(BK_final) and
cov flattened from cov_cart. method = "bk_native".
Initial lambda and Marquardt accept threshold match the Cartesian
fit at orbit_fit.cpp:553. Early-exit guard: returns a non-success
FitResult (flag = 1) without crashing when detections.size() < 3.
main.cpp gains an orbit_fit::bk_fit_bindings(m) call alongside the
existing orbit_fit bindings, exposing run_bk_native_fit to Python.
tests/layup/test_bk_fit.py covers the Layer 2 smoke tests:
* binding loads and run_bk_native_fit returns a FitResult
* empty-obs path returns flag != 0 without crashing
* <3 obs path triggers the early-exit guard
Layer 2 convergence tests against synthetic observations from a
known orbit (and the Cartesian/BK agreement test on well-arced
mainbelt) are next steps -- they need either the predict-path
output piped back in or the diagnostic/scan dataset wired up to
this branch.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generate synthetic observations from a known Cartesian state via
layup's predict_sequence path (a fixed barycenter observer, so the
only dynamical content is the orbit itself), then feed those
observations back into both run_bk_native_fit and the existing
Cartesian fit.
Three new test categories on top of the smoke tests:
* test_bk_native_fit_recovers_known_state: with the truth state as
the seed, BK converges in essentially one iteration to a fit
state matching the truth to rtol=1e-6 and chi2 < 1e-12.
Parameterized over a 3 AU mainbelt 60-day arc and a 40 AU TNO
300-day arc.
* test_bk_native_fit_recovers_from_perturbed_seed: with the seed
perturbed by 0.1% in each component, the LM loop still converges
to the truth state (rtol=1e-6) -- exercising the chain-rule
Jacobian + Marquardt damping on a non-trivial number of
iterations. Same two orbital regimes.
* test_bk_and_cartesian_fits_agree: for the well-constrained
mainbelt case, run_bk_native_fit and run_from_vector_with_initial_guess
converge to states that agree at rtol=1e-6. Establishes the
"no regression on the easy case" baseline.
All seven tests in tests/layup/test_bk_fit.py pass with ASSIST
ephemeris available; skip cleanly without it.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wires the universal BK fitter (run_bk_native_fit) into layup's Python
do_fit pipeline alongside the existing Cartesian fit, so callers can
choose the engine per call rather than via the C++ entry point.
Changes:
- src/layup/orbitfit.py
* Import run_bk_native_fit from layup.routines.
* Add a module-level _MU_SUN constant (heliocentric GM in
AU^3 / day^2) used to construct the BK fixed energy prior.
* Add _run_fit(assist_ephem, initial_guess, obs, engine) helper
that dispatches to:
- run_from_vector_with_initial_guess for engine='cartesian'
- run_bk_native_fit (with _MU_SUN) for engine='bk_native'
- ValueError otherwise
* do_fit gains an `engine='cartesian'` parameter (default
preserves the existing behavior). All five
run_from_vector_with_initial_guess call sites inside do_fit
are now routed through _run_fit so the engine choice
propagates uniformly.
- tests/layup/test_bk_fit.py
* test_run_fit_dispatch_cartesian: _run_fit(..., 'cartesian')
matches direct run_from_vector_with_initial_guess on
synthetic mainbelt observations.
* test_run_fit_dispatch_bk_native: _run_fit(..., 'bk_native')
matches direct run_bk_native_fit(ephem, ig, obs, MU_SUN).
* test_run_fit_dispatch_unknown_engine_raises: ValueError on
an unknown engine name.
The 'auto' (distance-dispatched) engine from PR 323 is intentionally
not wired up here; when 323 lands first this branch rebases and
gains both options. Likewise the 'bk' (liborbfit-backed) engine
from PR 323 is independent of this work.
All 10 tests in test_bk_fit.py and 25 tests in test_bk_basis.py
continue to pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tests/layup/test_bk_everywhere.py drives both engine='cartesian' and
engine='bk_native' against the diagnostic/scan dataset at
~/Dropbox/claude_layup/diagnostic/scan/truth/ -- the same 7-population,
14-arc-length scan that PR 323's auto-dispatch was validated against.
Skips when either the ASSIST ephemeris or the diagnostic scan is
unavailable, so CI is unaffected.
Two test groups:
* test_engine_sweep_well_arced_cases: on long-arc cases (30-60d
mainbelt + 60d classical TNO), both engines converge near truth
(drift < 1% of heliocentric distance) and agree with each other.
* test_bk_beats_cartesian_on_short_arc_distant: on distant short-arc
cases (70 AU scattered / 42 AU classical at 10-14 day arcs), BK
drifts no more than Cartesian from truth AND uses fewer LM
iterations. This is the regime BK was designed for, and the
diagnostic data shows it strongly: on scattered_70AU_arc_014.00d
BK stays 0.02 AU from truth in 6 iterations while Cartesian
wanders 4.5 AU over 58 iterations.
The module also exposes a sweep_cases_from_diagnostic() helper for
ad-hoc engine-sweep harness scripts.
All 6 Layer 3 tests pass (in addition to the 25 Layer 1 + 10 Layer 2
tests, for 41 total BK tests on this branch).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A runnable CLI script that drives both engine='cartesian' and
engine='bk_native' across an entire diagnostic-scan directory, writes
per-case metrics to CSV, and prints a population-level summary
(BK wins / Cartesian wins / per-engine failures / mean iteration
counts, plus median+mean drift and iteration ratios).
Usage:
python tools/bk_engine_sweep.py --scan-dir <dir> --output <csv>
Defaults discover the project's diagnostic scan at
~/Dropbox/claude_layup/diagnostic/scan/truth and the layup ephemeris
cache at ~/Library/Caches/layup. Both are overrideable via flags,
so anyone with a compatible truth dataset can reproduce.
Running on the 98-case scan (truth state as LM seed, sigma_arcsec=0.1):
Population n BK win Cart win cart fail bk fail both fail
-------------------------------------------------------------------------------
centaur_15AU 14 13 0 0 0 1
centaur_25AU 14 9 2 2 0 1
classical_42AU 14 10 3 0 0 1
mainbelt_2.5AU 14 10 3 0 0 1
mainbelt_3.5AU 14 12 1 0 0 1
scattered_70AU 14 7 2 4 0 1
sednoid_80AU 14 6 1 6 0 1
-------------------------------------------------------------------------------
TOTAL 98 67 12 12 0 7
Across 79 cases where both engines succeed:
drift ratio (BK / Cart): median=0.560, mean=187.199
iter ratio (BK / Cart): median=0.386, mean=0.524
Headline: BK never fails when Cartesian succeeds (0 / 98), succeeds in
12 cases where Cartesian flag=2's out, and on the typical case is ~2x
closer to truth in ~40% the iterations. The mean drift ratio of 187 is
inflated by the 70-80 AU short-arc cases where Cartesian wanders 5-13 AU
while BK stays put.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
matthewholman
force-pushed
the
feat/bk-everywhere
branch
from
27cd722 to
b8dd571
Compare
Collaborator
Author
|
Heads up: CI is failing here at the "Install dependencies" step, but the failure is an upstream dependency issue, not anything in this PR. Root cause: ABI mismatch between the latest Reproduces locally on a clean clone today (2026-05-15). Affects every branch, not just this one — The 41 BK tests in this PR all pass locally against the previously-installed Suggested fix (separate from this PR, since it's a repo-wide problem): [build-system]
requires = [
"scikit-build-core>=0.10",
"pybind11",
"assist>=1.2,<1.3", # or whichever range is known-compatible
"rebound>=4.6,<4.7",
] |
Pure whitespace -- two blank-line adjustments black wants for PEP 8 spacing. No test changes; 25 tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 17, 2026
The BK test suites skipped in CI for two independent reasons, both in
the test guards rather than the engine:
1. The ephemeris guard hardcoded the macOS cache path
(~/Library/Caches/layup). On the Linux CI legs the real cache is
~/.cache/layup, so EPHEM_AVAILABLE was always False and every suite
skipped -- even though `layup bootstrap` had downloaded the files.
Resolve the path with pooch.os_cache("layup"), exactly as the
library does when it writes them, so the guard is correct on macOS
and Linux alike.
2. test_bk_everywhere gated on a personal absolute path
(~/Dropbox/.../diagnostic/scan/truth) that existed on no CI runner
and no other contributor's machine. Ship that truth set in-repo
(98 cases, ~756 KB) under tests/data/bk_scan_truth/ and load it via
a path relative to the test file.
Both guards plus the diagnostic-data loader now live in a shared
tests/layup/_bk_guards.py so the stacked BK-IOD / iod=auto suites can
reuse them. With the ephemeris present, the two suites this branch
owns go from all-skipped to 16 passing (41 across the BK directory).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kjnapier
reviewed
Jun 18, 2026
| { | ||
| // Internal cached quantities at the BK position (alpha, beta). | ||
| // | ||
| // p = n0 + alpha*a + beta*b |
Collaborator
There was a problem hiding this comment.
The notation here is a bit non-standard, it may be good to note that p, n0, a, and b are vectors, and say what they actually are.
kjnapier
reviewed
Jun 18, 2026
kjnapier
reviewed
Jun 18, 2026
kjnapier
requested changes
Jun 18, 2026
kjnapier
left a comment
kjnapier
left a comment
Collaborator
There was a problem hiding this comment.
Everything here looks good to me, modulo a few comments that I have left, mostly on clarification, and one correctness check on BK -> XYZ. I think it's right, but the units of the final velocity look strange so I would like a second set of eyes on it.
Under `pytest -n auto`, each xdist worker let OpenBLAS (numpy/scipy) and OpenMP spin up a full threadpool; N workers x N threads oversubscribes the ~4-core GitHub Linux runners. With the BK suites no longer skipped, that surfaced as the pytest step hanging >1h on Linux CI even though every individual orbit fit completes in <1s (verified by per-case timing: 196 fits in 10.4s, slowest 0.85s -- no grinding fit exists). Set OMP/OPENBLAS/MKL/NUMEXPR/VECLIB _NUM_THREADS=1 (via setdefault, so explicit settings still win) before numpy/REBOUND import, keeping total threads ~= cores. macOS (Accelerate + spawn) never reproduced the hang. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 18, 2026
Address kjnapier's review comments on #326: - bk_basis.cpp: state that p/n0/a/b/rho_hat and the tangent vectors are 3-vectors and what the fiducial basis is. - bk_basis.cpp: derive v as d/dt of r = rho_hat/gamma and add a dimensional check showing both terms are velocities (the funny-looking magnitudes come from rho_hat_alpha/beta not being unit length). - bk_fit.cpp: note the initial Marquardt lambda and accept threshold are empirical, copied from the Cartesian fitter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address kjnapier's review on #328 ('a lot of data files to store in the repo'): replace the 98 separate, pretty-printed bk_scan_truth/*.json files with one minified tests/data/bk_scan_truth.json keyed by case stem. Each case keeps only the fields the BK test suites actually read -- sigma_arcsec, epoch_jd_tdb, truth_state_at_epoch and per-observation {ra, dec, jd_tdb, observer_state_AU} -- dropping the unused diagnostic fields (truth_state_at_obs, ra_true_deg/dec_true_deg, rho_AU, provID, rms*, etc.). Net effect: 98 files/602 KB -> 1 file/135 KB, same test behavior. _bk_guards.load_diagnostic_case/diagnostic_case_names now read from the single file (cached via lru_cache). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
matthewholman
added a commit
that referenced
this pull request
Jun 20, 2026
…ture) Address Little-Ryugu's review on #327: test_bk_everywhere.py (and test_bk_fit.py) skipped on GitHub CI and on other contributors' machines because they hardcoded the macOS ASSIST cache path (~/Library/Caches/layup) and, for the Layer-3 sweep, a personal absolute truth-set path (~/Dropbox/claude_layup/diagnostic/scan/truth). Port the same fix already on #326/#331 onto this branch: - Add tests/layup/_bk_guards.py: resolves the ephemeris via pooch.os_cache (correct on macOS and Linux) and loads the diagnostic-scan truth set. - Ship the truth set in-repo as the consolidated, minified tests/data/bk_scan_truth.json (98 cases, 135 KB, one file). - Re-point test_bk_everywhere.py and test_bk_fit.py at those guards; test bodies are unchanged. Now only requires_ephem skips, on machines that haven't run `layup bootstrap`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
With exactly 3 observations the BK fit is exactly determined: ndof = 2*3 - 6 = 0. The quality gate computed result.csq / ndof, i.e. csq / 0: for a converged fit with csq > 0 (the gdot bound-orbit prior makes the residual nonzero even at 3 obs) this is +inf, so "csq/ndof > thresh" was always true and the fit was spuriously downgraded to flag 2. Skip the gate when ndof == 0 (an exactly-determined fit has no reduced-chi2 to test). Add a regression test over the 3-observation diagnostic cases asserting ndof == 0 fits keep flag 0; it fails (flag 2) without the guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses the review request to verify the units/correctness of the Cartesian velocity returned by bk_to_cartesian. The existing round-trip and analytic-Jacobian-vs-FD tests are necessary but indirect for this question: they show the forward/inverse maps are mutual inverses and that dcart_dbk is consistent with the map, but not directly that v is the time-derivative of r. This test advances the BK angular coordinates along their rates (alpha += adot*t, beta += bdot*t, gamma += gdot*t) and central-differences the position part of bk_to_cartesian, then checks it reproduces the velocity part. It confirms v = d/dt(rho_hat/gamma) is a genuine velocity in AU/day across the mainbelt/NEO/TNO cases; a sign error in the gdot term fails it. The intermediate rho_hat_alpha/beta tangent vectors are deliberately not unit length, which is why those terms look unusual while the units are consistent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mschwamb
previously requested changes
Jun 21, 2026
| Centralizes two things that the BK tests (Layers 2 and 3, plus the | ||
| BK-IOD and iod='auto' suites that stack on top) used to each hardcode: | ||
|
|
||
| 1. **The ASSIST ephemeris location.** Layup resolves its cache with |
Collaborator
There was a problem hiding this comment.
Does this need to be in as comments? it's really long text from the AI that reads as what changed not helpful for someone who needs to maintain this. Can this be condensed to what's useful for long term maintenance?
Collaborator
Author
There was a problem hiding this comment.
This has been shortened.
|
|
||
| 2. **The diagnostic-scan truth set.** These ASSIST-integrated truth | ||
| cases used to live at a personal absolute path | ||
| (``~/Dropbox/claude_layup/diagnostic/scan/truth``) that existed on no |
Collaborator
There was a problem hiding this comment.
Does this need to be in as comments? it's really long text from the AI that reads as what changed not helpful for someone who needs to maintain this. Can this be condensed to what's useful for long term maintenance?
Collaborator
Author
There was a problem hiding this comment.
This has also been shortened.
| that manifested as the test step hanging for well over an hour even | ||
| though every individual orbit fit completes in under a second -- see the | ||
| per-case timing in the BK test-skip investigation. One thread per | ||
| worker keeps the total thread count ~= core count and removes the hang. |
Collaborator
There was a problem hiding this comment.
Does this need to be in as comments? it's really long text from the AI that reads as what changed not helpful for someone who needs to maintain this. Can this be condensed to what's useful for long term maintenance?
Collaborator
Author
There was a problem hiding this comment.
Same with this one.
Trim the _bk_guards.py and conftest.py docstrings/comments from
change-narrative ("used to hardcode", "previously skipped", size
deltas, investigation references) down to what a maintainer needs:
what the guards/loaders provide and why the threadpool pinning and
pooch-derived cache path are required. Per review feedback on #326.
No behavior change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Brings the branch up to date with main (36 commits: the multi-root IOD picker + residual prefilter, IAS15 adaptive_mode=2, #339-#344 fixes, docs/dash_ui). Resolves the one conflict in src/layup/orbitfit.py, where bk-everywhere had added the engine= dispatch to the pre-picker do_fit while main rewrote do_fit around the multi-root picker. Resolution keeps main's picker/prefilter pipeline and threads `engine` through it: - _run_fit() gains iter_max; it is forwarded to the Cartesian engine (run_from_vector_with_initial_guess, which main parameterized) and ignored by bk_native (run_bk_native_fit takes mu and uses its own internal LM cap). - do_fit() keeps all of main's picker knobs and adds engine="cartesian". - the two picker LM passes and the incremental-fit loop now dispatch through _run_fit(..., engine, iter_max). Validated in an isolated build: 54/54 tests pass across test_bk_basis, test_bk_fit, test_bk_everywhere, and test_orbit_fit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@mschwamb, I believe your request for removing some of the comments has been addressed. Could check and re-approve if everything is order? |
Brings in #344 (percentile IOD prefilter + keep-best guard) so the 609631 macOS real-data validation passes. Clean merge — #344's changes are in iod.py, orthogonal to the bk_native engine work here. do_fit calls filter_candidates_by_residual without residual_percentile, so it uses the new 80th-percentile default. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
matthewholman
dismissed
mschwamb’s stale review
Dismissing this stale review: the requested change (condense the long AI-generated comments in tests/layup/_bk_guards.py and conftest.py to maintenance-useful text) was addressed in 356ad86 and replied to on each thread. The branch has since been brought up to date with main and CI is green (6/6). Dismissing to unblock the merge; @mschwamb please re-open or comment if anything's still unaddressed.
Collaborator
|
Sorry on a plane today so I couldn't get to any pull requests. I see this is merged. Thanks @kjnapier for the review |
Collaborator
Author
|
Thanks, Meg. I just added you as a reviewer of another PR.
…On Mon, Jun 22, 2026 at 10:17 PM Meg Schwamb ***@***.***> wrote:
*mschwamb* left a comment (Smithsonian/layup#326)
<#326 (comment)>
Sorry on a plane today so I couldn't get to any pull requests. I see this
is merged. Thanks @kjnapier <https://github.com/kjnapier> for the review
—
Reply to this email directly, view it on GitHub
<#326?email_source=notifications&email_token=AA5CVWPZWRWAGUQIIBM44335BHSD3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINZXGUYDCMBQHE22M4TFMFZW63VMON2GC5DFL5RWQYLOM5S2KZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-4775010095>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA5CVWOCK3FW75LMOJ6WV2L5BHSD3AVCNFSNUABFKJSXA33TNF2G64TZHM4TCNRSHAYTGMRUHNEXG43VMU5TINBVG4YTOOJRGUY2C5QC>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/AA5CVWMJ4M2MYU6EATFT3CT5BHSD3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINZXGUYDCMBQHE22M4TFMFZW63VMON2GC5DFL5RWQYLOM5S2KZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/AA5CVWOWZTMUZJFGGXXUAY35BHSD3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINZXGUYDCMBQHE22M4TFMFZW63VMON2GC5DFL5RWQYLOM5S2KZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
--
Matthew J. Holman, PhD
Senior Astrophysicist
Center for Astrophysics | Harvard & Smithsonian
60 Garden Street, MS #51
Cambridge, MA 02138
(617) 496-7775
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bernstein–Khushalani (BK) orbit-fit stack — reviewer overview
This set of PRs adds a Bernstein–Khushalani parameterized fit engine to
do_fit, alongside the existing Cartesian engine, plus a matching linear IODand an automatic IOD fallback. The BK parameterization is better-conditioned for
distant / short-arc objects (TNOs, Centaurs, scattered/sednoid orbits), where a
Cartesian Gauss-seeded fit is prone to failing or needing a good initial guess.
Review order (stacked bottom-up)
feat/bk-everywhereengine="bk_native") indo_fit, BK basis + Jacobian,run_bk_native_fitfeat/bk-iodrun_bk_iod) + a sweep showing BK-IOD and Gauss are complementaryfeat/iod-auto-fallbackiod="auto": try Gauss first, fall back to BK-IOD when every Gauss root fails to seed the LM#326 → #328 → #331 is a true git stack (each branch contains its parent's
commits), but all three PR against
main. So #328's and #331's diffs vsmaininclude the parent changes — please review bottom-up (#326 first, then only
each PR's delta).
Two related PRs are independent (against
main, mergeable in any order):feat/cli-engine-flag--engine {cartesian,bk_native}on thelayup orbitfitCLI (becomes useful once #326 lands)fix/weight-data-name-mismatchweight_data=TrueNameError+ an arcsec/radian unit bugTest & CI status
All five PRs are green on CI (Linux + macOS, Python 3.13/3.14). Getting there
also fixed the BK test suites' CI plumbing — previously they were silently
skipping in CI:
pooch.os_cache("layup")(thesame call the library uses to write it), so the guard is correct on Linux CI,
not just macOS. The ASSIST-integrated diagnostic truth set is vendored
in-repo (
tests/data/bk_scan_truth/, ~756 KB) so the Layer-3 tests runanywhere, not only on a developer machine.
tests/layup/conftest.pypins native math threadpools to one thread(
OMP/OPENBLAS/MKL/... = 1) sopytest -n autodoesn't oversubscribe the~4-core CI runners (that was hanging the Linux legs).
LAYUP_SLOW_TESTS: theiod="auto"recovery test runs a real LM fit on a degenerate distant orbit,whose ASSIST/IAS15 integration can grind for a very long time nondeterministically
on Linux. It's skipped by default (CI keeps the two fast
iod="auto"tests)and runs on demand with
LAYUP_SLOW_TESTS=1.Deferred / out of scope
ephemeris time bound, on long-period comets / hyperbolic / very-distant-short-arc
orbits) is a pre-existing ASSIST-side issue, documented separately for a future
fix (detect-and-defer the runaway integration). It is not introduced by this
stack — it was merely made visible once the BK tests started running.
fix/comet-test-subset, independent of this stack) subsets the slow369-comet
test_apply_cometby default (LAYUP_SLOW_TESTS=1runs the fullcatalogue), addressing the same grind in the pre-existing comet test.