Phase 5: Body disc + body blob techniques#116
Merged
Conversation
Add the two body-side NavTechniques that don't fit the DT-fitting shape: full-disc NCC correlation and brightness-weighted-moment centroid fitting. After this phase, body-fills-FOV, body-mostly-off-frame-with-irregular-shape, and unresolved-body scenes can navigate end-to-end through the orchestrator. BodyDiscCorrelateNav fuses every BODY_DISC feature's template into a single composite via Z-buffer paint (closer body's nonzero pixels overwrite farther body's), then runs the existing pyramid kpeaks NCC with use_gradient='auto' so the technique self-selects raw vs gradient mode per image. Multi-body composites improve disambiguation because the correlation peak's SNR grows roughly as sqrt(N) and the joint geometric constraint removes the swap-moon ambiguity solo correlation suffers from. BodyBlobNav computes a brightness-weighted-moment centroid for each predicted bbox, then fits a single 2-D translation that maps the predicted centroids onto the observed centroids in least-squares. Per-blob weight is the inverse of the centroid CRLB variance: N_lit * SNR^2 / radius^2. Confidence is intrinsically capped at 0.4 since a brightness-weighted centroid is a much weaker observation than a limb fit. Body extractor emission gate is now aligned with the design's default config values: LIMB_ARC_MAX_UNCERTAINTY_PX = 3.0 (was 2.0) and a new BODY_BLOB_MIN_DIAMETER_PX = 8.0 floor. Per-body shape overrides (gas giants, irregular satellites) can only push the floor upward, never downward. navigate_with_pyramid_kpeaks gains a backwards-compatible 'top_k_peaks' field exposing per-peak telemetry (sorted by quality descending) so BodyDiscCorrelateNav can populate peak_to_runner_up_ratio without re-running the correlation. Also adds 'used_gradient' to the result dict so the auto picker's choice is recorded honestly in BodyDiscDiagnostics. NavModelBody and NavModelBodySimulated now ship bbox-sized postage-stamp BODY_DISC templates (compose_template_features expects this contract; previous extfov-shaped buffers would have made the Z-buffer paint slice the wrong region the first time it ran on a real BODY_DISC feature). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pin today's behavior on the four operator-curated Phase 5 sidecars
(body_full_fov, body_partial_overflow, multi_body,
below_resolution_body), fix the BodyLimbNav / BodyTerminatorNav
confidence specs to hard-zero on spurious results, and tighten the
per-image logger-section context so unraisable-exception cleanup
warnings stop leaking out of pytest workers.
The ``hard_zero_if={'spurious': True}`` addition mirrors the
existing gate on BodyDiscCorrelateNav. Without it, a degenerate
LM run that returns ``rms_px = 0`` (default value on a zero-inlier
result) would feed the ``-alpha * rms_px`` term with the
artificial-perfect 0 and the formula would report high confidence
on a clearly-spurious result. Caught when the operator's
Rhea-partial-overflow integration sidecar (N1484593951) recorded
BodyTerminatorNav at confidence 0.903 with 0/895 inliers.
The CLI-driver pytest cleanup wraps each ``IMAGE_LOGGER.open(...)``
section in a ``try/finally`` that closes the per-image file handler
once the section exits. Without it, pytest's gc-driven unraisable-
exception detector reports a ResourceWarning for every per-image
log file that the worker did not close before the gc cycle, polluting
test output and tripping ``filterwarnings = ["error"]`` on the
strict-config CI matrix.
PHASE5_LIBRARY_SEED.md Scenario C now has a prominent caveat
explaining that BodyBlobNav requires an irregular body
(high ellipsoid_residual_km), not just a small-in-pixels regular
moon — the body extractor's gate is shape-uncertainty-based, not
size-based. The previous wording sent the operator down a
false-trail path on Mimas.
AUTONAV_PLAN.md gains a new "Final Phase-5 check matrix" plus
the post-seeding follow-up list (LIMB_ARC reliability formula
too punitive on fully-lit limbs, BodyDiscCorrelateNav
``consistency_tol`` too tight on auto-gradient mode-switch
scenes, BodyTerminatorNav coarse-NCC fragile in multi-body
crescent geometry, ensemble ``agreement_gap`` calibration).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughPhase 5 is marked shipped: two new body navigation techniques (BodyDiscCorrelateNav, BodyBlobNav) are added, body-feature emission gating/defaults are adjusted, correlation/fitting APIs now return peak/gradient telemetry, logging handler cleanup is hardened, and comprehensive tests plus image-library sidecars are included. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 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 47 minutes and 7 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/nav/support/correlate.py (1)
498-513:⚠️ Potential issue | 🟡 MinorValidate
use_gradientexplicitly at the API boundary.Both helpers currently accept any truthy/falsey value and silently coerce it into gradient/raw mode. With
'auto'now part of the contract, a typo like"false"or"Auto"takes the wrong search path instead of failing fast.Suggested fix
def navigate_single_scale_kpeaks( @@ use_gradient: bool = False, ) -> dict[str, Any]: + if not isinstance(use_gradient, bool): + raise TypeError( + f'use_gradient must be a bool for navigate_single_scale_kpeaks, got ' + f'{type(use_gradient).__name__}' + ) + """ @@ def navigate_with_pyramid_kpeaks( @@ use_gradient: bool | Literal['auto'] = False, logger: PdsLogger | None = None, ) -> dict[str, Any]: + if use_gradient not in (False, True, 'auto'): + raise ValueError( + "use_gradient must be one of False, True, or 'auto'" + ) + """TODO Clean this upAs per coding guidelines, "Validate inputs at the public API boundary of the library. Raise clear
ValueErrororTypeErrorexceptions for invalid arguments."Also applies to: 641-657
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/support/correlate.py` around lines 498 - 513, The public API parameter use_gradient in navigate_single_scale_kpeaks (and the sibling helper around lines 641-657) must be validated explicitly: ensure it only accepts True, False, or the string 'auto' (case-sensitive), otherwise raise a TypeError (or ValueError for invalid string) at the start of the function; update both navigate_single_scale_kpeaks and the other helper to check isinstance/use explicit equality, reject truthy/falsy non-bool strings like "false" or "Auto", and only then branch into gradient/raw/auto logic so incorrect inputs fail fast with a clear error message referencing the parameter name.src/nav/nav_model/nav_model_body.py (1)
603-623:⚠️ Potential issue | 🟠 MajorKeep
BODY_BLOBgated on high limb uncertainty.The new
elif self._predicted_diameter_px >= blob_min_pxbranch fires wheneverLIMB_ARCwas not emitted, not just whenlimb_uncertainty_px > LIMB_ARC_MAX_UNCERTAINTY_PX. That broadens the rule from an uncertainty fallback into a generic fallback and can route scenes with missing limb samples intoBodyBlobNavunexpectedly.Suggested fix
- elif self._predicted_diameter_px >= blob_min_px: + elif ( + limb_uncertainty_px > LIMB_ARC_MAX_UNCERTAINTY_PX + and self._predicted_diameter_px >= blob_min_px + ): features.append(self._build_blob_feature(shape))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/nav_model/nav_model_body.py` around lines 603 - 623, The BODY_BLOB branch is firing whenever LIMB_ARC wasn't emitted, not only when limb_uncertainty_px exceeds LIMB_ARC_MAX_UNCERTAINTY_PX; change the elif that checks self._predicted_diameter_px >= blob_min_px to also require high limb uncertainty so BODY_BLOB remains an uncertainty fallback. Concretely, update the condition to something like: elif limb_uncertainty_px > LIMB_ARC_MAX_UNCERTAINTY_PX and self._predicted_diameter_px >= blob_min_px, leaving the _build_blob_feature and existing limb sampling checks (_limb_sampler, _build_limb_arc, limb_arc_emitted) unchanged.
🤖 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 2924-2931: The Phase-5 binding text for
navigate_with_pyramid_kpeaks omits the returned top_k_peaks value even though
later logic uses it to compute peak_to_runner_up_ratio; update the binding
bullet for navigate_with_pyramid_kpeaks to explicitly document that it returns
both 'used_gradient': bool and 'top_k_peaks': list (or similar structure) so
implementers know top_k_peaks is part of the API contract and can be used to
populate peak_to_runner_up_ratio.
- Around line 1302-1313: The documentation for BodyDiscDiagnostics is missing
the peak_to_runner_up_ratio field; update the AUTONAV_PLAN description of
nav.nav_technique.BodyDiscCorrelateNav to state that BodyDiscDiagnostics is
populated with ncc_peak, consistency_px, used_gradient, body_count and
peak_to_runner_up_ratio (derived from the top_k_peaks returned by
navigate_with_pyramid_kpeaks), and ensure any mention of the pyramid wrapper or
nav.support.correlate notes that top_k_peaks is used to compute
peak_to_runner_up_ratio so implementers wire the diagnostic correctly.
In `@docs/developer_guide_techniques.rst`:
- Around line 224-235: Update the docs to match the shipped implementation:
change the consistency_px description to state it is the maximum Euclidean drift
across pyramid levels (not per-axis) as computed in
navigate_with_pyramid_kpeaks(), replace the "NCC Hessian" covariance wording
with that the wrapper forwards evaluate_candidate()'s Fisher covariance, and
correct the BodyBlob sample (sigmoid_arg) to reflect the actual summed
contributions per the code (or explain any additional offset/normalization
applied) so documentation values and terminology exactly match
src/nav/support/correlate.py and the confidence math used by BodyBlob and the
pyramid wrapper.
In `@PHASE5_LIBRARY_SEED.md`:
- Around line 35-44: The fenced pseudocode block lacks a language tag which
triggers markdownlint MD040; update the fence around the pseudocode (the block
containing limb_uncertainty_px, LIMB_ARC, BODY_DISC, BODY_BLOB, etc.) to include
a language tag (e.g., "text") so the opening ``` becomes ```text to satisfy the
linter while leaving the pseudocode content unchanged.
In `@src/nav/nav_technique/nav_technique_body_blob.py`:
- Around line 386-391: The at_edge logic should also catch fits that lie outside
the search window, so change the condition that computes at_edge (currently
using abs(fit.dv - margin_v) and abs(fit.du - margin_u) comparisons) to instead
compare absolute offsets against the margin minus tolerance: mark at_edge true
when abs(fit.dv) >= margin_v - AT_EDGE_TOLERANCE_PX or abs(fit.du) >= margin_u -
AT_EDGE_TOLERANCE_PX (preserving both u and v checks), so any fit beyond the
allowed window is treated as “at edge” and will be zeroed out.
- Around line 380-382: Validate context.image_noise_sigma at this API boundary:
if context.image_noise_sigma is None or <= 0, raise a ValueError describing the
invalid image_noise_sigma; do not coerce non-positive values to 1e-9 here. Keep
the float conversion for valid inputs (assign to noise_sigma) and pass that to
_collect_per_blob_residuals(eligible, image_ext, noise_sigma, self.logger),
leaving any tiny-epsilon guards for lower-level numeric code only.
- Around line 460-467: The _eligible_blobs function currently accepts any
positive predicted_diameter_px which can let subpixel blobs dominate downstream
weighting; modify _eligible_blobs to enforce the same minimum diameter used by
the weight model (predicted_diameter_px >= 8) by filtering out BodyBlobGeometry
blobs with predicted_diameter_px < 8, and additionally add defensive validation
at the public API boundary (raise ValueError if callers supply BodyBlobGeometry
with non-numeric or negative predicted_diameter_px). Ensure references to
BodyBlobGeometry and predicted_diameter_px remain in the checks so undersized
blobs are rejected (or alternatively clamp the radius before weight computation
in the weight function if preserving the blob is required).
- Around line 301-309: The function currently returns the scatter of individual
blob offsets (var_v/var_u computed as sum(weights * residuals^2) / total_weight)
instead of the variance of the fitted translation estimate; change the estimator
to use the variance of the weighted mean by dividing by total_weight**2 (i.e.,
compute var_v = max(float(np.sum(weights * residuals_v * residuals_v) /
(total_weight**2)), floor) and same for var_u) so the returned covariance for
the translation (the diag created at the end) correctly shrinks with more blobs;
keep the existing floor and dtype handling.
In `@src/nav/nav_technique/nav_technique_body_disc.py`:
- Around line 264-269: The _upsample_factor method must validate
correlation_fft_upsample_factor before returning it: retrieve the raw value from
getattr(self.config.offset, 'correlation_fft_upsample_factor', None); if it's
None return default 128; otherwise ensure it's an integer (reject booleans) and
strictly positive—if not, raise a ValueError with a clear message referencing
correlation_fft_upsample_factor; finally return the integer value. Use the
symbols _upsample_factor, self.config, offset_block, and
correlation_fft_upsample_factor to locate and update the code.
- Around line 186-193: In navigate() (in src/nav/nav_technique_body_disc.py)
validate the result of _filter_disc_features(features) and raise a ValueError if
the resulting eligible list is empty (i.e., no template-backed BODY_DISC
features) before calling compose_template_features; update the navigate()
docstring to document this ValueError for the case of empty eligible features
and reference BODY_DISC/template-backed requirements so callers get a clear
input error instead of a downstream composition/correlation failure.
In `@src/nav/support/correlate.py`:
- Around line 625-633: The function currently assigns a sorted list of candidate
dicts to winner['all_candidates'], creating a self-referential structure because
winner is an element of candidates; instead, create a new dict or shallow copy
for the return value (e.g., copy_of_winner = dict(winner)) and attach a separate
list that contains copies (or the original dicts) to
copy_of_winner['all_candidates'] so that none of the entries in all_candidates
reference the returned object; update the return to return copy_of_winner
(references: winner, candidates, all_candidates).
In
`@tests/integration/image_library/images/body_full_fov/N1572105349_1_CALIB.yaml`:
- Around line 69-70: The fixture's sidecar constraints currently allow
BodyBlobNav to run because techniques_must_skip omits it; update the sidecar
YAML so techniques_must_skip includes BodyBlobNav (or otherwise enumerate all
non-allowed techniques) while keeping techniques_must_run set to
[BodyDiscCorrelateNav] so only BodyDiscCorrelateNav is permitted to run; modify
the keys techniques_must_run and techniques_must_skip in the sidecar for the
N1572105349_1_CALIB entry to explicitly exclude BodyBlobNav.
In `@tests/nav/nav_model/test_nav_model_body_integration.py`:
- Around line 229-240: The test function
test_to_features_limb_uncertainty_at_threshold and its docstring claim the body
is "exactly at the LIMB_ARC threshold" but the code builds two cases around the
threshold using _build_body (km_per_pixel_at_limb=17.0 and 15.0) and asserts
presence/absence of NavFeatureType.LIMB_ARC; update the test name and docstring
to accurately state it verifies behavior on both sides of the
LIMB_ARC_MAX_UNCERTAINTY_PX threshold (e.g., "around" or "either side of" the
threshold) and mention the specific km_per_pixel_at_limb values used and the
expected branches (LIMB_ARC vs blob) so the intent of saturn_model.to_features
is clear.
In `@tests/nav/nav_technique/test_nav_technique_body_blob.py`:
- Around line 139-143: Add concise docstrings to each of the three test
functions that currently lack them (including
test_body_blob_infeasible_on_empty_input and the two other test functions
nearby) describing the test purpose and expected behavior; update each function
so its first statement is a short docstring summarizing what it verifies (e.g.,
"Verify that BodyBlobNav.is_feasible returns infeasible and
'no_body_blob_features' in reason for empty input").
- Around line 21-57: Replace the one-line docstring on _make_blob_feature with a
Google-style docstring that documents the function contract: describe that it
builds and returns a NavFeature of type BODY_BLOB whose bbox tightly bounds the
predicted disc, list Parameters: body_name (str), predicted_center_vu
(tuple[float, float]), predicted_diameter_px (float), bbox_pad (int, default 4)
and note how radius, bbox_extfov_vu, sigma_centroid/cov, and flags
(BodyBlobFlags) are derived, and add a Returns: NavFeature description
explaining key populated fields (geometry.bbox_extfov_vu,
geometry.predicted_center_vu, position_cov_px, reliability, usable_types).
- Around line 146-168: Update test_body_blob_infeasible_on_zero_diameter to
assert the specific infeasibility cause in addition to report.feasible being
False: after obtaining report from BodyBlobNav().is_feasible([feature]) add an
assertion that the report's failure reason field (e.g., report.reason or
report.reasons/failure_reasons as used by the feasibility Report object)
contains a clear marker like "predicted_diameter", "diameter", or "zero" (or
otherwise exactly matches the message emitted when predicted_diameter_px == 0.0)
so the test fails only when the zero-diameter check is not the reason for
infeasibility.
- Around line 170-194: Update the test_body_blob_confidence_capped_at_0_4 to
assert that the cap is actually engaged instead of merely not exceeded: after
calling BodyBlobNav().navigate(...) check that result.confidence equals the cap
(0.4) within a tiny tolerance (e.g., 1e-12) by replacing the current <=
assertion with a precise equality-with-epsilon assertion on result.confidence so
the test fails if confidence drops below the cap.
In `@tests/nav/nav_technique/test_nav_technique_body_disc.py`:
- Around line 245-289: Update the two tests to assert deterministic numeric
diagnostics instead of loose sign/range checks: in
test_body_disc_diagnostics_records_peak_to_runner_up_ratio (using
BodyDiscCorrelateNav.navigate and result.diagnostics.peak_to_runner_up_ratio)
replace the >1.0 check with an assertion against the current expected numeric
value using pytest.approx (choose a tight tolerance, e.g., rel=1e-3). Similarly
in test_body_disc_diagnostics_records_consistency_and_quality assert exact
expected values for result.diagnostics.ncc_peak and
result.diagnostics.consistency_px with pytest.approx (tight tolerances). Run the
tests locally to capture the present expected numbers and update the assertions
to those values so the tests are deterministic.
---
Outside diff comments:
In `@src/nav/nav_model/nav_model_body.py`:
- Around line 603-623: The BODY_BLOB branch is firing whenever LIMB_ARC wasn't
emitted, not only when limb_uncertainty_px exceeds LIMB_ARC_MAX_UNCERTAINTY_PX;
change the elif that checks self._predicted_diameter_px >= blob_min_px to also
require high limb uncertainty so BODY_BLOB remains an uncertainty fallback.
Concretely, update the condition to something like: elif limb_uncertainty_px >
LIMB_ARC_MAX_UNCERTAINTY_PX and self._predicted_diameter_px >= blob_min_px,
leaving the _build_blob_feature and existing limb sampling checks
(_limb_sampler, _build_limb_arc, limb_arc_emitted) unchanged.
In `@src/nav/support/correlate.py`:
- Around line 498-513: The public API parameter use_gradient in
navigate_single_scale_kpeaks (and the sibling helper around lines 641-657) must
be validated explicitly: ensure it only accepts True, False, or the string
'auto' (case-sensitive), otherwise raise a TypeError (or ValueError for invalid
string) at the start of the function; update both navigate_single_scale_kpeaks
and the other helper to check isinstance/use explicit equality, reject
truthy/falsy non-bool strings like "false" or "Auto", and only then branch into
gradient/raw/auto logic so incorrect inputs fail fast with a clear error message
referencing the parameter name.
🪄 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: 59c6dc26-08b0-4b45-8982-8a3d180e3361
📒 Files selected for processing (28)
AUTONAV_PLAN.mdPHASE5_LIBRARY_SEED.mddocs/developer_guide_techniques.rstsrc/main/nav_mosaic.pysrc/main/nav_mosaic_cloud_tasks.pysrc/main/nav_offset.pysrc/nav/nav_model/body_shape.pysrc/nav/nav_model/nav_model_body.pysrc/nav/nav_model/nav_model_body_simulated.pysrc/nav/nav_technique/__init__.pysrc/nav/nav_technique/dt_fitting.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/navigate_image_files.pysrc/nav/support/correlate.pytests/integration/image_library/images/below_resolution_body/N1777325846_1_CALIB.yamltests/integration/image_library/images/body_full_fov/N1572105349_1_CALIB.yamltests/integration/image_library/images/body_partial_overflow/N1484593951_2_CALIB.yamltests/integration/image_library/images/multi_body/N1487595731_1_CALIB.yamltests/nav/nav_model/test_body_shape.pytests/nav/nav_model/test_nav_model_body.pytests/nav/nav_model/test_nav_model_body_integration.pytests/nav/nav_orchestrator/test_orchestrator.pytests/nav/nav_technique/test_nav_technique_body_blob.pytests/nav/nav_technique/test_nav_technique_body_disc.pytests/nav/support/test_correlate.py
* BodyBlob _joint_covariance: divide weighted-residual sum by total_weight^2 so the diagonal is 1/sum(w) inflated by residual scatter, matching the docstring. * BodyBlob at_edge now flags fits beyond the search window (|fit.dv| >= margin_v - tol) instead of only a thin band around the boundary; the moment-based offset is unbounded so beyond-window fits must zero out. * navigate_single_scale_kpeaks returns a copy of the winner with all_candidates attached separately, breaking the prior self-reference (winner was an entry inside its own all_candidates list). * Doc accuracy: AUTONAV_PLAN now lists peak_to_runner_up_ratio in BodyDiscDiagnostics and documents that navigate_with_pyramid_kpeaks returns top_k_peaks; developer guide describes consistency_px as the maximum Euclidean drift across pyramid levels and points the BodyDisc covariance at fisher_covariance() inside evaluate_candidate(); BodyBlob breakdown sample matches the three real spec terms. * Tests: BodyBlob zero-diameter test asserts the predicted_diameter reason; cap test asserts confidence == 0.4 exactly; missing docstrings added; LIMB_ARC threshold test docstring rewritten to describe the around-the-threshold check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nav/support/correlate.py (1)
616-625:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the no-candidate result shape consistent.
When no peaks survive, this fallback omits
peak_val, butnavigate_with_pyramid_kpeaks()unconditionally logsres_lvl["peak_val"]on Line 903. Any pyramid level with zero candidates will raiseKeyErrorinstead of returning a spurious result.Proposed fix
return { 'offset': (0.0, 0.0), 'cov': np.diag([1e6, 1e6]), 'sigma_xy': (1e3, 1e3), 'quality': -np.inf, + 'peak_val': float('nan'), 'all_candidates': [], }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/support/correlate.py` around lines 616 - 625, The fallback return when no candidates are found is missing the peak_val key which causes a KeyError in navigate_with_pyramid_kpeaks() when it unconditionally accesses res_lvl["peak_val"]; update the no-candidates branch in the function that builds the result dict (the dict shown in correlate.py) to include a peak_val entry (use a sensible numeric sentinel such as 0.0 or np.nan to match the expected float type), so the result shape matches successful cases and downstream logging of res_lvl["peak_val"] no longer fails.
♻️ Duplicate comments (1)
tests/nav/nav_technique/test_nav_technique_body_blob.py (1)
21-28: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winExpand helper docstring to full Google-style contract.
_make_blob_featurestill uses a single-line docstring; this helper should document parameters and return shape explicitly.Proposed update
def _make_blob_feature( body_name: str, *, predicted_center_vu: tuple[float, float], predicted_diameter_px: float, bbox_pad: int = 4, ) -> NavFeature: - """Build a BODY_BLOB feature whose bbox tightly bounds the predicted disc.""" + """Build a synthetic BODY_BLOB feature for BodyBlobNav tests. + + Parameters: + body_name: Synthetic body identifier embedded in the feature id and flags. + predicted_center_vu: Predicted blob center in (v, u) pixels. + predicted_diameter_px: Predicted blob diameter in pixels. + bbox_pad: Extra pixel padding added around the predicted disc bounds. + + Returns: + A NavFeature configured as BODY_BLOB with predicted geometry, bbox, + covariance, reliability breakdown, and BODY_BLOB usability metadata. + """As per coding guidelines, "ALWAYS include a docstring for every module, class, function, and method" and "Follow PEP 257 using Google style. Use
Parameters:...Returns:...".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/nav/nav_technique/test_nav_technique_body_blob.py` around lines 21 - 28, Update the single-line docstring for _make_blob_feature to a full Google-style docstring: add a short summary, a Parameters: section documenting body_name (str), predicted_center_vu (tuple[float, float]) as the predicted (v, u) center, predicted_diameter_px (float), and bbox_pad (int, default 4) with their meanings, and a Returns: section stating it returns a NavFeature representing a BODY_BLOB with a bounding box that tightly bounds the predicted disc (describe coordinate/frame expectations if applicable). Keep the function name _make_blob_feature and the return type NavFeature in the docstring so callers can locate behavior easily.
🤖 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 2690-2693: Update the overview phase status table to match the
Phase 5 section title by changing Phase 5's status from "Pending" to "Shipped"
(or "Complete") so it aligns with the header "Phase 5 — Body disc + body blob
techniques (complete)"; locate the table entry for Phase 5 in AUTONAV_PLAN.md
(the overview phase table referenced near the top) and replace the current
status cell for Phase 5 with the same final-status text used in the Phase 5
section heading.
In `@src/nav/nav_technique/nav_technique_body_blob.py`:
- Around line 480-486: In _search_window_for_obs (NavContext), remove the silent
fallback to (32,32) and validate context.obs.extfov_margin_vu: if the attribute
is missing or None, raise a ValueError; if present, ensure it is an
iterable/sequence of length 2 with numeric elements and convert them to ints
before returning; if the shape or types are wrong raise a ValueError (or
TypeError for non-numeric types) with a clear message referencing
extfov_margin_vu so callers can detect malformed NavContext instead of silently
guessing a window.
In `@tests/nav/nav_model/test_nav_model_body_integration.py`:
- Around line 237-242: Replace the Unicode em-dashes in the docstring bullet
points that describe the km_per_pixel_at_limb cases with ASCII hyphens;
specifically update the lines describing "km_per_pixel_at_limb = 17.0
(uncertainty ... below threshold) - saturn_model.to_features must emit a
LIMB_ARC" and "km_per_pixel_at_limb = 15.0 (uncertainty ... above threshold) -
the limb branch must drop and the technique falls through to BODY_BLOB" so the
em dashes (`—`) are changed to simple hyphens (`-`) in the docstring in
test_nav_model_body_integration.py.
In `@tests/nav/nav_technique/test_nav_technique_body_blob.py`:
- Around line 84-86: The comment containing a Unicode em dash should be changed
to use ASCII punctuation: replace the em dash in the line above the assertion in
test_nav_technique_body_blob.py (the comment referencing the hard cap at 0.4 and
that the technique cannot dominate the ensemble) with a normal hyphen/minus (-)
so the comment contains only ASCII characters; ensure the rest of the comment
text and the assertion using result.confidence remain unchanged.
---
Outside diff comments:
In `@src/nav/support/correlate.py`:
- Around line 616-625: The fallback return when no candidates are found is
missing the peak_val key which causes a KeyError in
navigate_with_pyramid_kpeaks() when it unconditionally accesses
res_lvl["peak_val"]; update the no-candidates branch in the function that builds
the result dict (the dict shown in correlate.py) to include a peak_val entry
(use a sensible numeric sentinel such as 0.0 or np.nan to match the expected
float type), so the result shape matches successful cases and downstream logging
of res_lvl["peak_val"] no longer fails.
---
Duplicate comments:
In `@tests/nav/nav_technique/test_nav_technique_body_blob.py`:
- Around line 21-28: Update the single-line docstring for _make_blob_feature to
a full Google-style docstring: add a short summary, a Parameters: section
documenting body_name (str), predicted_center_vu (tuple[float, float]) as the
predicted (v, u) center, predicted_diameter_px (float), and bbox_pad (int,
default 4) with their meanings, and a Returns: section stating it returns a
NavFeature representing a BODY_BLOB with a bounding box that tightly bounds the
predicted disc (describe coordinate/frame expectations if applicable). Keep the
function name _make_blob_feature and the return type NavFeature in the docstring
so callers can locate behavior easily.
🪄 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: f1308d26-b5a9-46a3-8542-ac21741bb676
📒 Files selected for processing (6)
AUTONAV_PLAN.mddocs/developer_guide_techniques.rstsrc/nav/nav_technique/nav_technique_body_blob.pysrc/nav/support/correlate.pytests/nav/nav_model/test_nav_model_body_integration.pytests/nav/nav_technique/test_nav_technique_body_blob.py
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## rf_core_rewrite #116 +/- ##
==================================================
Coverage ? 54.78%
==================================================
Files ? 129
Lines ? 15574
Branches ? 2104
==================================================
Hits ? 8532
Misses ? 6454
Partials ? 588 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
* AUTONAV_PLAN overview table: Phase 5 row updated from "Pending" to "Complete (branch core_rewrite_phase5)" so it agrees with the Phase 5 section header further down. * nav_technique_body_blob._search_window_for_obs: drop the unreachable getattr(obs, 'extfov_margin_vu', None) -> (32, 32) fallback. ObsSnapshot.__init__ always sets the property to a validated (v, u) tuple so the None branch can never run. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 30, 2026
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
BodyDiscCorrelateNav,BodyBlobNav) that don't fit the DT-fitting shape, plus the supporting body-extractor emission gate, postage-stamp BODY_DISC templates, glob-negation extension verification, four operator-curated integration sidecars, and a per-image logger-cleanup that stopsResourceWarningleakage out of pytest workers.AUTONAV_PLAN.md).Changes
BodyDiscCorrelateNav(src/nav/nav_technique/nav_technique_body_disc.py): full-disc NCC with Z-buffer paint multi-body fusion viacompose_template_features,use_gradient='auto'mode picker, andBodyDiscDiagnosticspopulated withncc_peak,consistency_px,used_gradient,body_count, andpeak_to_runner_up_ratio(derived from the newtop_k_peaksfield).BodyBlobNav(src/nav/nav_technique/nav_technique_body_blob.py): brightness-weighted-moment centroid joint translation across each blob's predicted bbox, per-blob CRLB weightsN_lit * SNR^2 / radius^2, diagonal joint covariance, intrinsic 0.4 confidence cap. Helpers (_collect_per_blob_residuals,_joint_offset_from_residuals,_joint_covariance) are module-level sonavigate()reads as straight assembly.LIMB_ARC_MAX_UNCERTAINTY_PXbumped from 2.0 to 3.0; newBODY_BLOB_MIN_DIAMETER_PX = 8.0floor; gate readsmax(global_floor, per_body_override)so the per-body table can only push the floor upward.NavModelBodyandNavModelBodySimulated:compose_template_featuresexpects bbox-sized templates and would slice the wrong region of memory the first time it ran on a real BODY_DISC feature with the previous extfov-shaped buffer; both call sites now.copy()the bbox slice.AT_EDGE_TOLERANCE_PXlifted tonav.nav_technique.dt_fitting: single source of truth across BodyLimbNav, BodyTerminatorNav, BodyBlobNav (and any future translation-fit techniques).navigate_with_pyramid_kpeaksextended (backwards-compatible) with'used_gradient': booland'top_k_peaks': list[(quality, dv, du)]fields; the auto picker records its choice honestly and the per-peak telemetry feedspeak_to_runner_up_ratiowithout re-running the correlation.BodyLimbNav/BodyTerminatorNav: both confidence specs gainhard_zero_if={'spurious': True}(matching the pre-existing gate on BodyDiscCorrelateNav). Without it a degenerate LM that returnsrms_px = 0(default value on a zero-inlier result) feeds the-alpha * rms_pxterm with the artificial-perfect 0 and the formula reports high confidence on a clearly-spurious result. Caught by the operator's Rhea-partial-overflow integration sidecar (N1484593951) recording BodyTerminatorNav at confidence 0.903 with 0 / 895 inliers.IMAGE_LOGGER.open(...)block innav_offset.py,nav_mosaic*.py, andnavigate_image_files.pynow wraps intry/finallyso the per-image file handler closes on exceptional exits. Without it pytest's gc-driven unraisable-exception detector firesResourceWarningfor every per-image log file the worker did not close before its gc cycle, polluting test output and trippingfilterwarnings = ["error"]on the strict-config CI matrix._filter_modelsglob-negation extension verified: three new orchestrator tests intests/nav/nav_orchestrator/test_orchestrator.pypin the mixed include/exclude pattern behavior on bothonly_modelsandonly_techniques.PHASE5_LIBRARY_SEED.md): four candidate scenarios for new sidecars, per-scenario sidecar location and expected status / confidence_tier values, with a prominentellipsoid_residual_km), not just a small-in-pixels regular moon.tests/integration/image_library/images/body_full_fov/N1572105349_1_CALIB.yaml(Dione fully in FOV)tests/integration/image_library/images/body_partial_overflow/N1484593951_2_CALIB.yaml(Rhea partial overflow)tests/integration/image_library/images/multi_body/N1487595731_1_CALIB.yaml(Dione + Rhea Z-buffer paint)tests/integration/image_library/images/below_resolution_body/N1777325846_1_CALIB.yaml(Mimas at ~20 px diameter)docs/developer_guide_techniques.rstgains a "Body-disc and body-blob techniques" section, a "Body-extractor emission gate" subsection, a worked DEBUG-log confidence-breakdown sample for BodyBlobNav, and an extended "See also" block.AUTONAV_PLAN.mdPhase 5 section gains a "What shipped" subsection (recording every delivery) plus a "Phase 5 follow-ups" section listing four real bugs the new sidecars surfaced (LIMB_ARC reliability formula too punitive on fully-lit limbs; BodyDiscCorrelateNavconsistency_toltoo tight on auto-gradient mode-switch scenes; BodyTerminatorNav coarse-NCC fragile in multi-body crescent geometry; ensembleagreement_gapthreshold needs calibration).Type of Change
Testing
Unit tests pass
Integration tests pass (if applicable)
End-to-end tests pass (if applicable)
New or updated tests for changed code
Tested manually (describe below if applicable)
pytest -n auto --dist=loadfile: 1097 unit tests pass.pytest -n auto --dist=loadfile tests/integration/: 28 tests pass (4 new Phase-5 sidecars + 3 Phase-4 carry-overs + structural-invariants + baselines + schema unit tests).pytest --cov=nav.nav_technique:nav_technique_body_disc.py93 %,nav_technique_body_blob.py95 %.New tests added: 13 end-to-end tests for the two new techniques (planted-offset recovery, multi-body Z-buffer paint, infeasibility, at-edge, blank-image spurious fallback, 0.4 hard cap, registry presence, peak-to-runner-up, consistency / quality, residual / SNR), the
TestPyramidTopKPeaksclass intests/nav/support/test_correlate.py, and the three orchestrator mixed-pattern glob tests.Manual verification: each of the four new sidecars was hand-verified with
nav_offset --manual; the integration regression test pins today's per-technique offset / confidence / status values for each one.Potential Impacts
navigate_with_pyramid_kpeaksreturns two new fields (used_gradient,top_k_peaks); the existing fields are unchanged.BodyDiscCorrelateNavandBodyBlobNavare new public classes registered innav.nav_technique.__init__.py; no existing class changes its signature.AT_EDGE_TOLERANCE_PXmoved from per-technique private constants tonav.nav_technique.dt_fitting's__all__(public).compose_template_features's template-payload contract was always "bbox-sized postage stamp"; the body NavModels' previous extfov-sized buffers happened not to trip it because no Phase 4 sidecar emitted a BODY_DISC. External callers that already followed the contract are unaffected; any caller that relied on the previous incorrect behavior would have been broken on its first BODY_DISC anyway.BodyDiscCorrelateNavruns the kpeaks pyramid twice per call whenuse_gradient='auto'(once raw, once gradient) — same cost as the existingmanual_nav_dialog's auto pick. No new I/O, caches, or threading surface.RingAnnulusNav(Phase 6) will readtop_k_peaksfor the same ratio diagnostic; the convention is established here.Checklist
ruff check,ruff format)mypypassesNotes
core_rewrite_phase5:Phase 5: Body disc + body blob techniques(the techniques + gate + glob-negation work) andPhase 5: integration seeding + spurious-gate + pytest cleanup(the four operator-curated sidecars + the spurious-confidence-gate fix on BodyLimbNav / BodyTerminatorNav + the per-image logger-section pytest cleanup). Reviewing the net result againstrf_core_rewriteis the intended scope.AUTONAV_PLAN.mdare real bugs surfaced by the integration seeding; none block Phase 5 (every technique handles its documented happy and boundary paths) but they are concrete starting points for Phase 6 / Phase 10 calibration work.phase_05_review/(the per-phase critique reports) is intentionally left out of this PR per the project's untracked-critique convention; the resolution log records every Medium / Low fix applied during the in-flight review pass.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests