Phase 7+8 star techniques + manual-nav + body cleanup#120
Merged
Conversation
Phase 7 ships StarUniqueMatchNav (1- and 2-star unique-match) and StarRefineNav (pass-2 prior refinement); Phase 8 ships the StarFieldFromCatalogNav RANSAC pattern matcher with deterministic sort-based candidate iteration. After this branch every star scene has a feasibility envelope: 1-2 stars via unique-match, >=3 via field-match, prior-required refinement in pass 2. Branch-wide diff summary: - src/nav/nav_technique/_star_helpers.py: new shared submodule (star_features, usable_stars, predicted_snr, predicted_vu, brightness_margin_mag, local_centroid). - src/nav/nav_technique/nav_technique.py: new search_window_for_obs helper consumed by all 8 techniques (replaces 8 per-module copies). - src/nav/nav_technique/dt_fitting.py: AT_EDGE_TOLERANCE_PX module constant deleted; every technique reads at_edge_tolerance_px from config_510_techniques.yaml. - src/nav/feature/composition.py: STAR features now render as a bbox-sized rectangle outline in the manual-nav dialog overlay (fixes star-only scenes hitting no_renderable_features_for_manual_nav). - src/nav/nav_technique/nav_technique_manual.py: matching is_feasible branch for STAR features. - StarUniqueMatchNav.brightness_margin_mag now reports +inf when no third predictable star is available (consistent semantics across 1- and 2-star paths). - StarRefineNav: single_inlier_confidence_cap (default 0.5) prevents a 1-inlier refine from out-ranking a 1-star unique-match on the same observation. Operator runbook: PHASE7_LIBRARY_SEED.md covers the four scenarios (A bright single star, B optional two-star, C star-rich field, D stars+body) plus the manual-nav workflow. Two seed sidecars included: W1449079117_1_CALIB (Vega, scenario A) and W1580760393_1_CALIB (Pleiades, scenario C, currently expected.status=failed pending the Phase 10 SNR-units fix). AUTONAV_PLAN.md: Phase 7 + 8 marked complete; Phase 8 Sub-piece 4 re-worded to match the shipped translation-only Procrustes; Phase 10 scope F filed to track the calibrated_if SNR-units bug surfaced during W1580760393 curation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
WalkthroughMarks Phase 7 and Phase 8 complete and adds three STAR navigation techniques, expands StarFlags with per-star metadata, consolidates search-window logic, adds STAR visualization to overlays, introduces shared STAR helpers, updates configs/tunables, and adds comprehensive documentation, integration sidecars, and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## rf_core_rewrite #120 +/- ##
==================================================
Coverage ? 53.86%
==================================================
Files ? 135
Lines ? 16471
Branches ? 2230
==================================================
Hits ? 8872
Misses ? 7010
Partials ? 589 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AUTONAV_PLAN.md`:
- Around line 3576-3593: The Phase 10 STAR SNR block in AUTONAV_PLAN.md is using
an indented literal block (with "::") which triggers MD046; locate the "SNR =
sig / sqrt(sig + image_noise_sigma**2 * N_aperture)" equation in the Phase 10
section (the STAR SNR “SNR = …” block) and replace the indented/:: style literal
block with a fenced code block using triple backticks so the equation is
rendered consistently and markdownlint MD046 is satisfied; ensure the fence
encloses the exact equation line(s) and remove the leading indentation/::
marker.
In `@docs/user_guide_navigation.rst`:
- Around line 136-138: The docs currently list STAR techniques
(BodyTerminatorNav, RingAnnulusNav, RingEdgeNav, StarUniqueMatchNav,
StarRefineNav, StarFieldFromCatalogNav) as implemented earlier but then also
include them in the “Pending techniques (not yet shipped)” section; update the
latter to remove or reword any entries that reference those six STAR techniques
so the page is consistent (either delete those three STAR entries from the
pending list or change the pending section to only list genuinely unshipped
techniques and note that the STAR techniques are implemented).
In `@PHASE7_LIBRARY_SEED.md`:
- Line 276: Replace the hardcoded verified_date value in the sidecar template so
it doesn't ship stale metadata: update the verified_date entry currently set to
"2026-04-30" in PHASE7_LIBRARY_SEED.md to a placeholder such as "YYYY-MM-DD" or
a templated token like "{{VERIFIED_DATE}}" (the key to change is verified_date)
and document in the template comment how consumers should populate it.
- Line 246: The example test command uses shell-redirection syntax `<IMAGE_ID>`
which can be interpreted as stdin redirection; update the command string so the
placeholder is safe to copy-paste — for example replace `<IMAGE_ID>` with a
quoted placeholder like "IMAGE_ID" or a token without angle brackets (IMAGE_ID)
in the example (the command line shown: pytest
tests/integration/test_autonomous_nav.py -v -k <IMAGE_ID>) so shells will not
treat it as redirection.
In `@src/nav/feature/flags.py`:
- Around line 53-61: In __post_init__ of the dataclass (the method currently
validating smear_length_px and predicted_snr) add explicit type and finiteness
checks: for predicted_snr ensure it is an int/float but not bool (raise
TypeError if not numeric), then use math.isfinite to reject NaN/inf and raise
ValueError if non-finite or if predicted_snr < 0.0 (keep the existing negative
check but make its error a ValueError with a clear message). For vmag, if not
None validate similarly that it is numeric (not bool) and finite, raising
TypeError for wrong type and ValueError for non-finite values; keep existing
smear_length_px validation as-is. Ensure error messages reference the offending
field name and value.
In `@src/nav/nav_technique/_star_helpers.py`:
- Around line 86-105: The guard order in brightness_margin_mag is wrong: check
brightest_snr <= 0.0 before checking runner_up_snr so an unpopulated/zero
brightest SNR does not get treated as uniquely bright; modify the function
brightness_margin_mag to return 0.0 immediately when brightest_snr <= 0.0, and
only then treat runner_up_snr <= 0.0 as +inf, leaving the final return of 2.5 *
math.log10(brightest_snr / runner_up_snr) unchanged.
In `@src/nav/nav_technique/nav_technique_manual.py`:
- Around line 75-86: Update the run_manual_nav() docstring to reflect that STAR
features are now rendered as rectangle outlines: add STAR to the list of feature
kinds and describe that STAR paints a rectangle outline at the predicted VU
position sized by the per-feature PSF bbox (consistent with is_feasible()
behavior); also update the corresponding descriptive text in the other docstring
blocks referenced (the similar paragraphs around the other occurrences) so all
mentions include STAR rectangle overlays and match the wording used in
compose_dialog_overlay/is_feasible().
In `@src/nav/nav_technique/nav_technique_star_field.py`:
- Around line 190-208: The function that computes and returns the
similarity-invariant (r1, r2, theta) triple currently only rejects coincident
points but must also detect and reject collinear triplets: after forming vectors
v1 = pb - pa and v2 = pc - pa compute a 2D cross product (or sin(theta)) and if
its absolute value is below a small epsilon (or if computed theta is ≈0 or ≈π)
return None to drop the triplet; apply the same collinearity check in the second
occurrence of this logic (the block around the later 222-226 region) so
0/π-angle hashes are never produced for RANSAC.
In `@src/nav/nav_technique/nav_technique_star_unique_match.py`:
- Around line 221-245: The two local_centroid() results (det_a, det_b) can be
the same detection when search windows overlap; add a guard after those calls to
detect duplicate detections (e.g., compare a stable identifier or the measured
pixel positions/centroids from det_a vs det_b and/or the peak coordinates
peak_a/peak_b with a tiny distance tolerance) and if they refer to the same
observation, log a debug message and return None to force the one-star fallback;
update the check near chosen/ local_centroid to treat det_a==det_b (or distance
< epsilon) as equivalent to missing a second independent detection.
In `@src/nav/nav_technique/nav_technique.py`:
- Around line 35-46: search_window_for_obs currently reads
context.obs.extfov_margin_vu and silently truncates floats; update the function
to validate the input at the public boundary: check that context has obs and obs
has extfov_margin_vu, that extfov_margin_vu is a sequence of length 2, and that
both elements are integers >= 0; raise TypeError with a clear message if the
attribute is missing or elements are not int types, and raise ValueError if the
sequence length is wrong or any value is negative; keep the return as
tuple[int,int] and reference the NavContext and search_window_for_obs symbols in
your changes.
In `@tests/nav/feature/test_composition.py`:
- Around line 405-427: Update the
test_compose_dialog_overlay_renders_star_marker_rectangle test to assert the
exact set of nonzero pixels for the STAR outline rather than only checking
corners and center: compute the expected perimeter pixel coordinates (using the
star's predicted_vu and computed half-extent from bbox_pad -> half-width 5) and
assert that mask.nonzero() (and image nonzero positions/values) exactly equals
that expected perimeter set, and likewise tighten the similar STAR test around
the other test (the one referenced after this block) to mirror this exact-pixel
comparison; locate the assertions in the test function and replace the loose
corner/center checks with exact set equality checks against
compose_dialog_overlay output.
In `@tests/nav/nav_technique/conftest.py`:
- Around line 462-464: The test fixture is hardcoding
NavReliabilityBreakdown.predicted_snr=1.0 while _make_star_feature and
StarFlags.predicted_snr accept a predicted_snr parameter, causing inconsistent
STAR fixtures; update the reliability_reasons construction in conftest.py to
pass through the same predicted_snr value used to build the feature (e.g., use
the function/local parameter predicted_snr or StarFlags.predicted_snr when
constructing NavReliabilityBreakdown(predicted_snr=...)) so both
StarFlags.predicted_snr and NavReliabilityBreakdown.predicted_snr match.
In `@tests/nav/nav_technique/test_nav_technique_star_refine.py`:
- Line 120: The test currently masks failures by allowing either 'prior'
appearing in result.feature_ids or result.confidence == 0.0; make it
deterministic and focused on the failure contract by removing the string-based
branch and asserting the expected zero-confidence directly: replace the compound
assertion with a single assertion that result.confidence == 0.0, referencing the
existing result object and its confidence attribute to ensure the test only
validates the contract of a no-prior failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 078d1462-9a15-4db6-a2f9-8e1394c9e6dd
📒 Files selected for processing (36)
AUTONAV_PLAN.mdPHASE7_LIBRARY_SEED.mddocs/developer_guide_navigation_models_stars.rstdocs/developer_guide_techniques.rstdocs/introduction_configuration.rstdocs/user_guide_navigation.rstsrc/nav/config_files/config_510_techniques.yamlsrc/nav/feature/composition.pysrc/nav/feature/flags.pysrc/nav/nav_model/stars/nav_model_stars.pysrc/nav/nav_technique/__init__.pysrc/nav/nav_technique/_star_helpers.pysrc/nav/nav_technique/diagnostics.pysrc/nav/nav_technique/dt_fitting.pysrc/nav/nav_technique/nav_technique.pysrc/nav/nav_technique/nav_technique_body_blob.pysrc/nav/nav_technique/nav_technique_body_disc.pysrc/nav/nav_technique/nav_technique_body_limb.pysrc/nav/nav_technique/nav_technique_body_terminator.pysrc/nav/nav_technique/nav_technique_manual.pysrc/nav/nav_technique/nav_technique_ring_annulus.pysrc/nav/nav_technique/nav_technique_ring_edge.pysrc/nav/nav_technique/nav_technique_star_field.pysrc/nav/nav_technique/nav_technique_star_refine.pysrc/nav/nav_technique/nav_technique_star_unique_match.pytests/integration/image_library/images/one_bright_star_no_body/W1449079117_1_CALIB.yamltests/integration/image_library/images/star_dominated/W1580760393_1_CALIB.yamltests/nav/feature/test_composition.pytests/nav/nav_orchestrator/test_orchestrator.pytests/nav/nav_technique/conftest.pytests/nav/nav_technique/test_nav_technique_body_blob.pytests/nav/nav_technique/test_nav_technique_manual.pytests/nav/nav_technique/test_nav_technique_star_field.pytests/nav/nav_technique/test_nav_technique_star_refine.pytests/nav/nav_technique/test_nav_technique_star_shared.pytests/nav/nav_technique/test_nav_technique_star_unique_match.py
💤 Files with no reviewable changes (1)
- src/nav/nav_technique/dt_fitting.py
…doc drift Six review findings addressed (one rejected with rationale): - StarFieldFromCatalogNav._triplet_hash now rejects collinear-but-distinct triplets via a 2-D cross-product magnitude check (|cross| <= _COLLINEAR_REL_EPS * d_ab * d_ac). Previously such triplets produced theta = 0 or pi hashes that matched any other 0-or-pi hash regardless of geometric scale — a false-match trap for the RANSAC matcher. - StarUniqueMatchNav._try_two_star now falls back to the one-star path when the two local_centroid() calls return centroids less than 1 px apart (overlapping search windows can resolve the same matched-filter peak for both predictions, fabricating a zero-residual cross-check). - _star_helpers.brightness_margin_mag guard order flipped: brightest_snr <= 0 returns 0.0 first, then runner_up_snr <= 0 returns +inf. Previously a fully-unpopulated (0, 0) cohort was treated as "uniquely bright". - NavTechniqueManual class docstring and run_manual_nav() Returns section now list STAR rectangle overlays alongside templates / polylines / blobs (matches is_feasible() and compose_dialog_overlay). - user_guide_navigation.rst: deleted the stale "Pending techniques (not yet shipped)" subsection (all three star techniques now ship; the closing claim about no_feasible_techniques is the opposite of current behavior). - test_composition.py STAR-marker tests now assert the exact perimeter pixel set (matching the existing Bresenham-circle test pattern); a bug that paints only corners or fills the rectangle would now fail loudly. - test_star_refine_requires_prior: dropped the dead 'prior' in str(result.feature_ids) branch (feature_ids is the consumed-features tuple, not a failure reason); single result.confidence == 0.0 assertion remains. Rejected: a finding asking conftest._make_star_feature to pass predicted_snr through to NavReliabilityBreakdown. The two fields are deliberately different quantities — StarFlags.predicted_snr is the raw integrated SNR (uncapped) while NavReliabilityBreakdown.predicted_snr is documented as a [0, 1] normalized contribution score. Production code maps raw SNR through _snr_reason_score; passing the raw value would violate the field's documented range. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Purpose
Ship Phase 7 (
StarUniqueMatchNav,StarRefineNav) and Phase 8(
StarFieldFromCatalogNav) perAUTONAV_PLAN.mdso every star scene has afeasibility envelope: 1-2 stars via unique-match, ≥ 3 via field-match,
prior-required refinement in pass 2. Includes the manual-nav-dialog STAR
rendering fix surfaced during library curation, the
at_edge_tolerance_px/AT_EDGE_TOLERANCE_PXbody-technique cleanup, and two seed sidecars (Vega,Pleiades).
Changes
Phase 7 — Star unique-match + refine (new techniques):
nav_technique_star_unique_match.py— 1-star path (cap 0.7) and 2-starpath (cap 0.8) with brightness-margin gate; YAML tunables for search
window, centroid box, residual cap, per-mode confidence caps.
nav_technique_star_refine.py— pass-2 refinement on a pass-1 prior withper-star inverse-variance weighting, outlier drop, single-inlier
confidence cap (default 0.5) so a 1-inlier refine cannot dominate a
1-star unique-match on the same observation.
Phase 8 — Multi-star RANSAC pattern matcher:
nav_technique_star_field.py— four-stage pipeline (matched-filtersource detection, similarity-invariant triplet hashing, deterministic
RANSAC sorted-by-hash-distance + lex-sorted detection-source indices per
AUTONAV_PLAN §33, Tukey-biweight verification). Translation-only
Procrustes; rotation-enabled 3-DoF lights up in Phase 9.
enumeration / candidate ranking / translation solve / inlier counting.
(offset_px, covariance_px2, confidence, diagnostics)across two back-to-back invocations.Cross-technique cleanup (closes Phase 7+8 critique High findings):
nav.nav_technique._star_helperssubmodule consolidatesstar_features,usable_stars,predicted_snr,predicted_vu,brightness_margin_mag,local_centroid(was duplicated across threestar modules with cross-module private imports).
search_window_for_obspromoted tonav.nav_technique.nav_technique(was 8 identical per-module copies, body / ring / star).
AT_EDGE_TOLERANCE_PXconstant deleted fromdt_fitting; everytechnique reads
at_edge_tolerance_pxfromconfig_510_techniques.yaml.tuning(matchingRingEdgeNav's pattern).Manual-nav dialog STAR rendering (real bug surfaced during curation):
compose_dialog_overlaypaints a bbox-sized rectangle outline aroundevery STAR feature's predicted (v, u). Previously star-only scenes hit
no_renderable_features_for_manual_naveven though there was a star toalign — affected every Scenario A / B operator-curation workflow.
is_feasiblebranch inNavTechniqueManual.test_composition.py(rectangle painting, off-imageno-op, near-edge half-width clamp) plus a manual-nav test for
template-less STAR features.
Diagnostics polish:
StarUniqueMatchDiagnostics.brightness_margin_magreports+infwhenno third predictable star is available (was reporting the magnitude
difference between the two matched stars in the 2-star + no-third-star
case, which contradicted the field docstring).
Operator runbook + seed sidecars:
PHASE7_LIBRARY_SEED.mdcovers the four scenarios (A bright singlestar, B optional two-star, C star-rich field, D stars + body) plus the
manual-nav workflow. Scenario B is documented as optional — the
2-star path is structurally identical to 1-star and fully exercised by
the synthetic unit test.
W1449079117_1_CALIB(Vega, Scenario A) andW1580760393_1_CALIB(Pleiades, Scenario C). The Pleiades sidecarrecords
expected.status: failedpending the Phase 10F SNR-units fixfiled during curation.
Plan + docs updates:
AUTONAV_PLAN.md: Phase 7 + 8 marked complete with implementationnotes; Phase 8 Sub-piece 4 wording corrected to match the shipped
translation-only Procrustes; new Phase 10 scope F documents the
calibrated-IF SNR-unit-mismatch bug surfaced during W1580760393
curation.
developer_guide_techniques.rst: new "Star techniques" sectioncovering all three new techniques.
developer_guide_navigation_models_stars.rst,introduction_configuration.rst,user_guide_navigation.rst: updatedtechnique listings.
Type of Change
technique modules
Testing
pytest -n auto --dist=loadfile)end-to-end on this branch (no
PDS3_HOLDINGS_DIRenv in CI image)test_nav_technique_star_unique_match.py(7),_star_refine.py(8),_star_field.py(21),_star_shared.py(3 parameterized),test_composition.py(3 STAR-marker tests),test_nav_technique_manual.py(1 template-less STAR test, 1no-features test)
Save as Library Entry…exercised for both seed sidecars (Vega and Pleiades)Potential Impacts
Public API additions:
nav.nav_technique.StarUniqueMatchNav,StarRefineNav,StarFieldFromCatalogNav(re-exported fromnav.nav_technique.__init__).nav.nav_technique.diagnostics.StarUniqueMatchDiagnostics,StarRefineDiagnostics,StarFieldDiagnostics.nav.nav_technique.nav_technique.search_window_for_obs(new publichelper for technique authors).
nav.nav_technique._star_helpers(private submodule; not exported).Removed symbols:
nav.nav_technique.dt_fitting.AT_EDGE_TOLERANCE_PX— replaced by per-technique YAML
at_edge_tolerance_px. No callers outside thetechnique package.
Config schema additions (
config_510_techniques.yaml):techniques.StarUniqueMatchNav.*,StarRefineNav.*,StarFieldFromCatalogNav.*blocks added (placeholders pending Phase10 calibration sweep).
at_edge_tolerance_px: 1.0added under each body / ring technique'stuningblock.single_inlier_confidence_cap: 0.5added underStarRefineNav.tuning.Performance: none expected for downstream callers; the new techniques
are M^3 in catalog-and-detection counts but bounded at 30 by the
max_sourcescap (~27 K candidate triplets per side, ~10 ms perinvocation in the unit-test harness).
Backward compatibility: none broken. No public-API removals.
Checklist
ruff check,ruff format)mypy --strictpasses (282 source files)sphinx-build -W -b html docs docs/_buildclean)definition of done
Notes
Phase 10 follow-up filed during this branch. Curating
W1580760393_1_CALIB(Pleiades) for Scenario C surfaced a real bug:predicted_snr.integrated_signal_dnreturns DN-equivalent values, butimage_noise_sigmais in whatever units the obs delivered (DN for_RAW, I/F for_CALIB). For I/F images the SNR formula collapses to√sigand every star fails the reliability gate. Filed as Phase 10scope item F; the worked example and direction (per-instrument-per-
processing-level scale factor or convert
image_noise_sigmatoDN-equivalent at NavContext build time) are documented in
AUTONAV_PLAN.md. The Pleiades sidecar'sexpected.status: failedrecords honest current behavior; flip to
okonce Phase 10F lands.Vega sidecar
primary_technique. The W1449079117 sidecar pinsprimary_technique: StarRefineNavbecause that was the orchestrator'sactual output before the
single_inlier_confidence_capshipped on thisbranch. After the cap,
StarUniqueMatchNavshould win primary on a1-star scene. A follow-up commit (or the Phase 10 calibration pass)
should re-run
nav_offseton Vega and flip the sidecar back toStarUniqueMatchNavonce the new behavior is verified.Scenario B (two-star) deferred. Documented as optional in
PHASE7_LIBRARY_SEED.md— real frames with exactly 2 detectablecatalog stars are rare on Cassini ISS (NAC FOV too narrow, WAC
typically catches a third), and the 2-star path is fully exercised by
the synthetic unit test
test_star_unique_match_two_star_recovers_planted_offset.Summary by CodeRabbit
New Features
Documentation