Phase 6: Ring-annulus technique + per-technique config#117
Merged
Conversation
- Ship RingAnnulusNav: NCC against multi-ring composite RING_ANNULUS templates with use_gradient='auto', Z-buffer paint of multi-planet annulus features, and a full diagnostics + confidence-spec wiring matching the BodyDiscCorrelateNav shape. - Ship config_510_techniques.yaml carrying every NavTechnique's confidence-formula coefficients plus per-technique runtime tunables (spurious thresholds, at-edge tolerance, minimum arc length) plus the planet-specific feature_emission.ring_annulus block (Saturn / Jupiter / Uranus / Neptune). Module-level magic constants for those values are deleted; load_confidence_spec / load_technique_tuning validate the YAML at config-load time and raise ConfidenceConfigError on malformed input. - Add system-level RING_ANNULUS emission gate keyed on per-planet km/px threshold so distant-Saturn ring scenes (where the entire ring system spans only a few pixels) emit one annulus per surviving feature rather than per-edge polylines. - Add ring filter pass-4 outermost-preservation: at very low resolution every neighboring fade overlaps every other and the per-edge check could drop every outer-side edge; restore the feature carrying the largest in-range edge so navigation has at least one outer reference (typically the A-ring outer edge for Saturn). - Fix --nav-models 'rings' / 'body:saturn' glob normalization: bare prefixes expand to prefix:* and the value after a colon is uppercased to match the prefix:VALUE registry naming convention. - Fix pre-existing nav_model_body.py filter_downsample crash on tiny moons (JANUS / PANDORA): split the empty-silhouette path so a downsampled-shape km/pixel array is no longer fed back through filter_downsample. - Disable integration tests in CI via -m 'not integration': operator-curated image-library regression tests rely on real PDS3 holdings + SPICE kernels and are slow / flaky in CI. Run locally before merging. - Update docs/introduction_configuration.rst, docs/developer_guide_techniques.rst, and docs/user_guide_navigation.rst for the new config design and the shipped RingAnnulusNav. - New unit-test files: test_nav_technique_ring_annulus.py (9 tests), test_confidence_config.py (20 tests). Existing rings / orchestrator / body-technique test files extended to cover the planet-specific annulus gate, the outermost-preservation pass, and the model-glob normalization. - Add PHASE6_LIBRARY_SEED.md operator runbook and two new operator-verified sidecars for distant Saturn ring views (N1447064164, W1444747627). 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 (6)
WalkthroughAdds a new RingAnnulusNav technique, moves per-technique confidence coefficients and tuning into a YAML registry loaded at config-read time, refactors techniques to use config-backed tuning, updates ring-model emission/filtering to support composite annuli, adds tests/docs/sidecars, and adjusts CI to exclude integration tests by default. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 34 minutes and 38 seconds.Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## rf_core_rewrite #117 +/- ##
==================================================
Coverage ? 52.36%
==================================================
Files ? 131
Lines ? 15825
Branches ? 2161
==================================================
Hits ? 8286
Misses ? 6977
Partials ? 562 ☔ 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/nav/nav_technique/nav_technique_body_limb.py (1)
238-243: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueBodyLimbNav still uses
AT_EDGE_TOLERANCE_PXfromdt_fittingmodule.Unlike
RingEdgeNavwhich readsat_edge_tolerance_pxfrom its tuning config,BodyLimbNavstill uses the importedAT_EDGE_TOLERANCE_PXconstant fromdt_fittingat lines 239-242. This creates an inconsistency:RingEdgeNavhas a configurable at-edge tolerance whileBodyLimbNavandBodyTerminatorNavdo not.If this is intentional (shared DT-fitting tolerance vs. technique-specific), consider documenting why. Otherwise, consider adding
at_edge_tolerance_pxto theBodyLimbNav.tuningblock for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/nav_technique/nav_technique_body_limb.py` around lines 238 - 243, BodyLimbNav currently hardcodes AT_EDGE_TOLERANCE_PX from dt_fitting when computing the at_edge boolean; replace that with a tuning parameter so it matches RingEdgeNav (or document intention). Add an at_edge_tolerance_px entry to BodyLimbNav.tuning and read it where at_edge is computed (use self.tuning.at_edge_tolerance_px instead of AT_EDGE_TOLERANCE_PX), then use that value in the abs(dv_final +/- margin_v) and abs(du_final +/- margin_u) comparisons; also mirror the same change or add a note/update in BodyTerminatorNav if it should share the same behavior.src/nav/nav_technique/nav_technique_body_terminator.py (1)
208-213:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast in
navigate()when no eligible terminator features remain after filtering.
navigate()currently proceeds even ifeligible_featuresis empty, which can push empty arrays
into downstream fitting code and produce opaque runtime failures. Raise a clearValueErrorhere.Suggested patch
eligible_features = [ f for f in features if isinstance(f.geometry, TerminatorPolyline) and f.geometry.vertices_vu.shape[0] >= self._min_arc_px ] + if len(eligible_features) == 0: + raise ValueError( + 'BodyTerminatorNav.navigate() requires at least one eligible TERMINATOR_ARC ' + f'with >= {self._min_arc_px:g} vertices' + ) self.logger.info( 'Consuming %d TERMINATOR_ARC features (out of %d offered)', len(eligible_features), len(features), )As per coding guidelines: "Validate inputs at the public API boundary of the library. Raise clear
ValueErrororTypeErrorexceptions for invalid arguments."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/nav_technique/nav_technique_body_terminator.py` around lines 208 - 213, In navigate(), after building eligible_features (the list comprehension that filters features by isinstance(f.geometry, TerminatorPolyline) and f.geometry.vertices_vu.shape[0] >= self._min_arc_px), add a guard that checks if eligible_features is empty and immediately raises a ValueError with a clear message (e.g., "no eligible terminator features after filtering by TerminatorPolyline and min arc pixels"); this prevents empty arrays from being passed into downstream fitting code and surfaces a clear API error at the public boundary.AUTONAV_PLAN.md (1)
3059-3238:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse single-space sentence separation in the updated Phase 6 section.
Several changed lines use two spaces after a period (for example Line 3059 and Line 3188). Please normalize to one space for consistency and guideline compliance.
As per coding guidelines, "Always use one space between the period at the end of a sentence and the next sentence in documentation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AUTONAV_PLAN.md` around lines 3059 - 3238, The Phase 6 documentation contains sentences with double spaces after periods; find the updated "Phase 6" narrative (the "What shipped in Phase 6" section and the "Phase 6 follow-ups" paragraphs that mention RingAnnulusNav, config_510_techniques.yaml, and PHASE6_LIBRARY_SEED.md) and normalize sentence spacing to a single space after each period throughout that section; search for occurrences of " " (two spaces) following a period and replace them with a single space, verifying examples near mentions of RingAnnulusNav and the follow-ups are corrected.
🤖 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 3160-3174: The test count in AUTONAV_PLAN.md is inconsistent: the
summary claims the PR adds 20 tests but the detailed bullet lists show 9 tests
in tests/nav/nav_technique/test_nav_technique_ring_annulus.py and 13 in
tests/nav/nav_technique/test_confidence_config.py (total 22); update the
sentence that asserts "20 tests" to the correct total (22) or add a
parenthetical note clarifying that the documented 13 tests in
test_confidence_config.py represent a subset of the PR's additions—modify the
summary line that references the PR's declared additions and/or the sentence
mentioning test_confidence_config.py to make the counts consistent with the
listed files.
In `@PHASE6_LIBRARY_SEED.md`:
- Around line 267-269: Update the CI description: change the sentence that
claims "CI runs the `integration` mark on every PR" to reflect the current
policy that integration tests are operator/local by default and are excluded
from PR CI (we run pytest with -m 'not integration'); mention the `integration`
mark and the use of -m 'not integration' to make the behavior explicit.
- Around line 42-50: The fenced code block starting with "radial_extent_px = max
- min projection of the polyline onto its mean radial normal" is missing a
language tag; update that markdown block in PHASE6_LIBRARY_SEED.md by adding an
explicit language identifier (e.g., "text") after the opening backticks so it
reads "```text" to satisfy markdownlint MD040 and preserve the existing block
contents and closing backticks.
In `@src/nav/nav_model/rings/ring_filter.py`:
- Around line 217-233: The current restore logic may append outermost_feature
into result even when a feature with the same key already exists (possibly
trimmed), causing duplicates; modify the block around
survives_outermost/outermost_feature so that before appending you check result
for an existing feature with the same key (use outermost_feature.key) and
replace that entry (preserving position if needed) instead of blindly
result.append(...). Update the logic that computes survives_outermost and then
perform a key-based lookup/replace in result (using feature.key from items in
result and feat.all_base_radii()) to ensure only one entry per key.
In `@src/nav/nav_technique/nav_technique_body_terminator.py`:
- Around line 149-153: After reading config in the constructor
(self.config.read_config()) and parsing tuning entries into self._min_arc_px,
self._spurious_dt_rms_factor, self._spurious_dt_floor_px, and
self._spurious_min_inliers, add explicit validation that raises ValueError with
clear messages: verify each numeric is finite (use math.isfinite) and that
_min_arc_px > 0, _spurious_dt_rms_factor >= 0, _spurious_dt_floor_px >= 0, and
_spurious_min_inliers >= 0 and an integer; if any check fails raise ValueError
identifying the offending field and expected bounds so invalid config values
fail fast during initialization.
In `@src/nav/nav_technique/nav_technique_ring_annulus.py`:
- Around line 152-160: The navigate() flow does not guard against empty annulus
templates and should raise a clear error: after computing eligible =
_filter_annulus_features(features) (and before calling compose_template_features
or using template_img/template_mask), check if eligible is empty and raise a
ValueError with a descriptive message (e.g., "No usable RING_ANNULUS templates
available for navigation"); this prevents downstream failures in
compose_template_features and correlation logic and makes the public API
boundary validation explicit.
- Around line 229-235: The _upsample_factor method must validate the external
config value correlation_fft_upsample_factor from self.config.offset: ensure the
raw value exists, is a real number but not a bool, and when converted to int is
>=1 and below a reasonable upper bound (e.g. 1_000_000 or sys.maxsize/2) to
avoid overflow; on any invalid type or out-of-range value raise a clear
ValueError mentioning "correlation_fft_upsample_factor" and the offending value.
Locate method _upsample_factor, check getattr(self.config, 'offset', None) and
then inspect the raw value via getattr(offset_block,
'correlation_fft_upsample_factor', None), validate type with isinstance(...,
numbers.Real) and not isinstance(..., bool), convert to int safely, enforce
bounds, and raise ValueError with a descriptive message if validation fails.
- Around line 258-271: The function _search_window_for_obs currently assumes
context.obs.extfov_margin_vu is an indexable pair of non-negative numbers;
validate this before using it: fetch margin = getattr(obs, "extfov_margin_vu",
None), and if not None check that it is a sequence/tuple of length >=2, that
margin[0] and margin[1] are ints or floats, and that both are >= 0; if any check
fails raise a ValueError with a clear message (mention
NavContext.obs.extfov_margin_vu in the text); on success return (int(margin[0]),
int(margin[1])) and keep the existing (32, 32) fallback when margin is None.
In
`@tests/integration/image_library/images/ring_only_curved/N1447064164_1_CALIB.yaml`:
- Around line 29-36: The sidecar contains TODO placeholders in the Phase-6 note
and uses expected.status: conflicted and expected.confidence_tier: conflicted;
replace the TODO coordinates in the "Phase-6 status: RingAnnulusNav converges to
(TODO, TODO)" line with the actual converged pixel coordinates (from the latest
Phase-6 regression run) and set expected.status and expected.confidence_tier to
the true expected values, or if "conflicted" is intentionally correct, add a
one-line comment explaining why (e.g., "conflicted expected due to known
calibration drift in Phase-6") so the intent is clear for future reviewers;
update the YAML values for expected.status and expected.confidence_tier to match
that decision.
In
`@tests/integration/image_library/images/ring_only_curved/W1444747627_1_CALIB.yaml`:
- Around line 29-32: Update the committed operator-verified sidecar by removing
or replacing the placeholder TODOs in the Phase-6 status sentence: find the line
starting with "Phase-6 status: RingAnnulusNav converges to (TODO, TODO)," and
either insert the measured (x,y) convergence values (and the measured pixel
tolerance instead of "within TODO px") or remove that entire sentence if
measurements are not available; ensure the surrounding confidence/coefficient
text (e.g., "expected.confidence_tier") remains valid and that the file contains
no remaining TODO placeholders.
In `@tests/nav/nav_model/test_ring_filter.py`:
- Around line 503-507: The test currently only asserts membership with "'outer'
in keys" which can hide duplicates or extra survivors; replace that membership
check by asserting the exact expected keys sequence or set (e.g., assert keys ==
['outer'] or assert set(keys) == {'outer'}) depending on whether order matters,
targeting the local variables result and keys in the test function so the test
verifies the precise restored output rather than just existence.
---
Outside diff comments:
In `@AUTONAV_PLAN.md`:
- Around line 3059-3238: The Phase 6 documentation contains sentences with
double spaces after periods; find the updated "Phase 6" narrative (the "What
shipped in Phase 6" section and the "Phase 6 follow-ups" paragraphs that mention
RingAnnulusNav, config_510_techniques.yaml, and PHASE6_LIBRARY_SEED.md) and
normalize sentence spacing to a single space after each period throughout that
section; search for occurrences of " " (two spaces) following a period and
replace them with a single space, verifying examples near mentions of
RingAnnulusNav and the follow-ups are corrected.
In `@src/nav/nav_technique/nav_technique_body_limb.py`:
- Around line 238-243: BodyLimbNav currently hardcodes AT_EDGE_TOLERANCE_PX from
dt_fitting when computing the at_edge boolean; replace that with a tuning
parameter so it matches RingEdgeNav (or document intention). Add an
at_edge_tolerance_px entry to BodyLimbNav.tuning and read it where at_edge is
computed (use self.tuning.at_edge_tolerance_px instead of AT_EDGE_TOLERANCE_PX),
then use that value in the abs(dv_final +/- margin_v) and abs(du_final +/-
margin_u) comparisons; also mirror the same change or add a note/update in
BodyTerminatorNav if it should share the same behavior.
In `@src/nav/nav_technique/nav_technique_body_terminator.py`:
- Around line 208-213: In navigate(), after building eligible_features (the list
comprehension that filters features by isinstance(f.geometry,
TerminatorPolyline) and f.geometry.vertices_vu.shape[0] >= self._min_arc_px),
add a guard that checks if eligible_features is empty and immediately raises a
ValueError with a clear message (e.g., "no eligible terminator features after
filtering by TerminatorPolyline and min arc pixels"); this prevents empty arrays
from being passed into downstream fitting code and surfaces a clear API error at
the public boundary.
🪄 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: c905b0ec-d2b2-4144-80a9-a4478ce92dd2
📒 Files selected for processing (31)
.github/workflows/run-tests.ymlAUTONAV_PLAN.mdPHASE6_LIBRARY_SEED.mddocs/developer_guide_techniques.rstdocs/introduction_configuration.rstdocs/user_guide_navigation.rstsrc/nav/config/config.pysrc/nav/config_files/config_510_techniques.yamlsrc/nav/nav_model/nav_model_body.pysrc/nav/nav_model/nav_model_rings.pysrc/nav/nav_model/rings/ring_filter.pysrc/nav/nav_orchestrator/orchestrator.pysrc/nav/nav_technique/__init__.pysrc/nav/nav_technique/confidence_config.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_ring_annulus.pysrc/nav/nav_technique/nav_technique_ring_edge.pytests/integration/image_library/images/ring_only_curved/N1447064164_1_CALIB.yamltests/integration/image_library/images/ring_only_curved/W1444747627_1_CALIB.yamltests/nav/nav_model/test_nav_model_rings.pytests/nav/nav_model/test_nav_model_rings_integration.pytests/nav/nav_model/test_ring_filter.pytests/nav/nav_orchestrator/test_orchestrator.pytests/nav/nav_technique/test_confidence_config.pytests/nav/nav_technique/test_nav_technique_body_limb.pytests/nav/nav_technique/test_nav_technique_body_terminator.pytests/nav/nav_technique/test_nav_technique_ring_annulus.py
- Composite ring-annulus emission: collapse every annulus-eligible per-ring rendering into a single composite RING_ANNULUS per planet so the reliability formula's constituent_count term scales with the number of fused rings (was constituent_count=1 per feature, gating every annulus out at 0.135 < 0.30 default threshold). - _composite_ring_renderings helper: union per-ring masks (logical OR) and take the per-pixel max of the rendered images so overlapping ring contributions keep their brightest value. - Ring filter pass-4 outermost-preservation: replace the trimmed entry by key when the outermost feature is restored, instead of appending a duplicate entry. - RingAnnulusNav.navigate explicit empty-input validation: raise ValueError with a descriptive message when every input feature lacks a template payload, matching the public API boundary. - _upsample_factor validation: reject bool, non-numeric, non-finite, and out-of-range values for config.offset.correlation_fft_upsample_factor; bound at 1 .. 1_000_000 to prevent FFT memory blow-ups; module constants _DEFAULT_UPSAMPLE_FACTOR and _MAX_UPSAMPLE_FACTOR document the bounds. - _search_window_for_obs hard-fails on a missing extfov_margin_vu attribute across all six techniques (was a silent fallback to 32x32). Test fixtures already supply the attribute via FakeObs; an obs without it is a programming error. - Adaptive PNG whitepoint: scale the bright-pixel clip count to the 5% of detected outliers (median + 15*MAD threshold) instead of the fixed 0.1% of all pixels. Dark-sky images with a few bright stars no longer saturate every star to 255; body-fills-FOV scenes keep the original 0.1% behaviour. - Pytest excludes integration tests by default via addopts = ["-m", "not integration"]; pass -m "" or -m integration to opt back in. - scripts/run-all-checks.sh gains a -i / --integration flag and prints a one-line message indicating whether integration tests will run. - Move all per-technique magic constants (LIMB_MIN_ARC_PX, TERMINATOR_MIN_ARC_PX, SPURIOUS_*, _AT_EDGE_TOLERANCE_PX, RING_ANNULUS_MAX_RADIAL_PX, RING_ANNULUS_KMPP_THRESHOLD) out of Python module constants and into config_510_techniques.yaml under per-technique tuning blocks plus the per-planet feature_emission.ring_annulus block. Tests load values via cls.tuning rather than direct constant import. - Docs: developer_guide_navigation_models_rings.rst, developer_guide_techniques.rst, user_guide_navigation.rst describe the new per-planet annulus emission gates; CLAUDE.md and CONTRIBUTING.md document the integration-test marker convention. - Validate `vol_start` and `vol_end` arguments to DataSetPDS3 against the available volume list and raise a descriptive ValueError on a typo. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/run-tests.yml:
- Line 102: The conditional expression in the GitHub Actions job uses
"&&!github.event..." which is valid but hard to read; update the if condition
(the line starting with "if: matrix.os == 'ubuntu-latest' &&
matrix.python-version == '3.13' &&!github.event.pull_request.head.repo.fork") to
add a space after the logical AND so it reads "&&
!github.event.pull_request.head.repo.fork" for improved readability.
In `@src/nav/config/config.py`:
- Around line 204-212: The try/except in config.py swallows
ConfidenceConfigError for underscore-prefixed test techniques and uses continue,
which skips resetting cls.tuning and causes stale tuning to persist; change the
flow so that even when cls.name.startswith('_') you still call
load_technique_tuning (or explicitly set cls.tuning = {} when tuning block is
absent) after handling the ConfidenceConfigError—i.e., remove the continue or
ensure cls.tuning is assigned (via load_technique_tuning or an explicit empty
dict) when the name starts with '_' so tuning is always reset regardless of the
confidence-spec error.
- Around line 202-213: Merged per-technique overrides aren't applied after
update_config because the loader logic that sets NavTechnique subclasses'
confidence_spec and tuning (the loop using NavTechnique._registry with
load_confidence_spec and load_technique_tuning and ending with
validate_registered_confidence_specs()) only runs from read_config(); extract
that block into a reusable helper (e.g. _validate_registered_techniques()) that
iterates NavTechnique._registry, sets cls.confidence_spec via
load_confidence_spec(cls.name), handles ConfidenceConfigError for names starting
with '_' the same way, sets cls.tuning via load_technique_tuning, and calls
validate_registered_confidence_specs(); then call this helper from both
read_config() and update_config() immediately after merging self._config_dict so
updated YAML overrides take effect.
In `@src/nav/dataset/dataset_pds3.py`:
- Around line 570-576: The code currently validates vol_start and vol_end
against all_volume_names and computes vol_start_idx and vol_end_idx but does not
check for the case where both are provided and vol_start comes after vol_end;
add an explicit validation after computing vol_start_idx and vol_end_idx that if
vol_start is not None and vol_end is not None and vol_start_idx > vol_end_idx
then raise ValueError (e.g., "vol_start must not be after vol_end") so callers
get a clear error instead of silently returning no rows; locate this check near
the existing vol_start/vol_end handling that references all_volume_names,
vol_start_idx and vol_end_idx in dataset_pds3.py.
In `@src/nav/nav_model/rings/ring_filter.py`:
- Around line 214-225: The survival check for the outermost feature should match
the exact feature identity and edge label, not just the radius: when unpacking
the chosen triple from after_res_in_range (outermost_radius, edge_label,
outermost_feature), keep edge_label and change survives_outermost to verify that
some feat in result has a base radius/edge pair (r, lbl) equal to
(outermost_radius, edge_label) (i.e., iterate feat.all_base_radii() and match
both r and lbl) or alternatively match feat.key == outermost_feature.key
together with the same radius and edge label so you only consider the exact
edge/feature you selected.
In `@src/nav/nav_technique/nav_technique_ring_annulus.py`:
- Around line 120-122: RingAnnulusNav's constructor currently only calls
super().__init__ and never populates confidence_spec, which causes a cold
RingAnnulusNav().navigate(...) to hit the assert that expects confidence_spec;
fix by initializing confidence_spec during __init__: after
super().__init__(config=config) call the same config-read/populate step other
techniques use (e.g. call self.read_config() or set self.confidence_spec =
self.config.confidence_spec if config already contains it) so confidence_spec is
populated before navigate() is ever invoked; update RingAnnulusNav.__init__
accordingly.
In `@src/nav/navigate_image_files.py`:
- Around line 241-247: The docstring currently claims the bright-outlier
threshold is median + 6 * MAD but the implementation computes median + 15 * MAD;
update the docstring to state the correct threshold (median + 15 * MAD) so it
matches the code path that computes threshold = median + 15 * mad (the
variables/expressions around median, mad, and the 15 * mad multiplier used to
count/clip bright outliers). Ensure the docstring wording mirrors the
implementation (and tests) exactly or alternatively change the multiplier in the
implementation to 6 if you intend to keep the docstring — pick one consistent
value and make both the code (threshold calculation) and the explanatory
docstring match.
- Around line 292-293: The branch in apply_linear_gamma_stretch that currently
does "if white <= black: white = black + 1.0" should use nextafter to preserve
any remaining contrast: replace the +1.0 bump with a call to
math.nextafter(black, math.inf) (or numpy.nextafter(black, float('inf')) if the
project prefers numpy) so white becomes the next representable float above
black; also ensure math (or numpy) is imported in the module. This change
targets the variables white and black inside apply_linear_gamma_stretch and
preserves low-dynamic-range detail safely.
In `@tests/nav/nav_model/test_nav_model_rings_integration.py`:
- Around line 223-232: The test currently only checks for a RING_ANNULUS but
doesn’t verify that RING_EDGE features are suppressed; update the assertions
after calling model.to_features(...) (the features list produced by
model.to_features) to assert the exact expected features—either add assert
len(features) == 1 to ensure only the composite annulus is emitted or explicitly
assert that no feature has feature_type == NavFeatureType.RING_EDGE (e.g., check
that all(f.feature_type != NavFeatureType.RING_EDGE for f in features)); keep
the existing checks for RING_ANNULUS and its flags/mask.
In `@tests/nav/nav_model/test_nav_model_rings.py`:
- Around line 186-212: Update the
test_composite_ring_renderings_unions_masks_and_takes_max_image test so it
includes a true overlapping non-zero pixel between the two inputs and asserts
the per-pixel max behavior: modify img_a and img_b (used in the call to
_composite_ring_renderings) to share at least one coordinate with different
intensities (e.g., img_a higher or img_b higher), then add an assertion that
composite_img at that shared coordinate equals the brighter input value (using
pytest.approx) to ensure np.maximum-style behavior is enforced rather than
last-writer-wins.
🪄 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: 5e255875-46f8-470c-be2e-e39a46d0545a
📒 Files selected for processing (24)
.github/workflows/run-tests.ymlCONTRIBUTING.mddocs/developer_guide_navigation_models_rings.rstdocs/developer_guide_techniques.rstdocs/user_guide_navigation.rstpyproject.tomlscripts/run-all-checks.shsrc/nav/config/config.pysrc/nav/dataset/dataset_pds3.pysrc/nav/nav_model/nav_model_rings.pysrc/nav/nav_model/rings/ring_filter.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_ring_annulus.pysrc/nav/nav_technique/nav_technique_ring_edge.pysrc/nav/navigate_image_files.pytests/integration/image_library/images/ring_only_curved/W1444747627_1_CALIB.yamltests/nav/nav_model/test_nav_model_rings.pytests/nav/nav_model/test_nav_model_rings_integration.pytests/nav/nav_model/test_ring_filter.pytests/nav/nav_technique/test_nav_technique_ring_annulus.pytests/nav/test_navigate_image_files.py
Apply eight verified review findings: - config.update_config: validate registered techniques after merging when read_default=True so user YAML technique overrides take effect. - _validate_registered_techniques: always reset cls.tuning for underscore test techniques so a stale value cannot leak across config reloads. - dataset_pds3: raise ValueError when vol_start follows vol_end instead of silently yielding zero rows. - ring_filter outermost preservation: match feature key plus exact (radius, edge_label) instead of radius alone, so two distinct features sharing a radius cannot mask a dropped outer edge. - navigate_image_files quantile stretch: docstring now matches the median + 15 * MAD threshold the implementation uses. - navigate_image_files quantile stretch: use np.nextafter when white collapses to black so any LSB-level contrast survives the bump. - ring annulus integration test: assert exactly one composite feature emits, suppressing all RING_EDGE features. - composite ring rendering test: include an overlapping non-zero pixel with different intensities and assert the brighter input wins.
This was referenced Apr 30, 2026
5 tasks
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
Phase 6 of the autonav rewrite ships
RingAnnulusNavso distant Saturn / Uranus / Jupiter / Neptune ring scenes — where the entire ring system spans only a handful of pixels — finally have a viable navigation technique. Closes the design's "low-resolution ring annulus" path, retires every per-technique magic constant in favor ofconfig_510_techniques.yaml, and fixes two pre-existing crashes (tiny-moonfilter_downsampleassertion,--nav-models 'rings'glob mismatch) that surfaced while exercising the new path against real holdings.Changes
RingAnnulusNav: pyramid-NCC fit onRING_ANNULUSfeatures withuse_gradient='auto', Z-buffer paint of multi-planet annulus features, and a full diagnostics + confidence-spec wiring matching theBodyDiscCorrelateNavshape.RING_ANNULUSper planet (constituent_count = number of fused rings) so the reliability formula'sconstituent_countterm scales with the size of the ring system. A new_composite_ring_renderingshelper unions per-ring masks (logical OR) and takes the per-pixel max of the rendered images.feature_emission.ring_annulus.{default, planets.SATURN, JUPITER, URANUS, NEPTUNE}carrymax_radial_pxandkmpp_threshold; the per-polyline gate fires when a single edge compresses belowmax_radial_pxand the system-level gate fires when the entire ring system spans only a handful of pixels (km_per_pixel_radial >= kmpp_threshold).key— never appends a duplicate — so the orchestrator's per-feature accounting holds.config_510_techniques.yamlis the single source of truth for everyNavTechnique's confidence-formula coefficients (load_confidence_spec) and runtime tunables (load_technique_tuning) — spurious-detection thresholds, at-edge tolerance, minimum arc length, plus the per-planetfeature_emission.ring_annulusblock. Module-level Python constants for those values are deleted; missing fields raiseConfidenceConfigErrorat config-load time so a YAML typo fails fast at process startup.RingAnnulusNav.navigateraisesValueErrorwith a descriptive message when every input feature lacks a template payload, matching the public API boundary._upsample_factorvalidatesconfig.offset.correlation_fft_upsample_factor: rejects bool, non-numeric, non-finite, and out-of-range values; bounded at[1, 1_000_000]to prevent FFT memory blow-ups._search_window_for_obshard-fails on a missingextfov_margin_vuattribute across all six techniques (was a silent 32x32 fallback).nav_model_body.pyfilter_downsamplecrash on tiny moons (JANUS / PANDORA) fixed: split the empty-silhouette path so a downsampled-shape km/pixel array is no longer fed back throughfilter_downsample.--nav-models 'rings'/'body:saturn'glob normalization: bare prefixes expand toprefix:*and the value after a colon is uppercased to match theprefix:VALUEregistry naming convention.vol_start/vol_endarguments toDataSetPDS3against the available volume list and raise a descriptiveValueErroron a typo.addopts = ["-m", "not integration"];scripts/run-all-checks.shgains a-i/--integrationflag and prints a one-line message indicating whether integration tests will run. CI'srun-tests.ymladopts the same default.Type of Change
filter_downsamplecrash;--nav-models 'rings'glob mismatchRingAnnulusNavand per-planet annulus emissiondeveloper_guide_techniques.rst,developer_guide_navigation_models_rings.rst,user_guide_navigation.rst,introduction_configuration.rst,CLAUDE.md,CONTRIBUTING.md,PHASE6_LIBRARY_SEED.mdtest_nav_technique_ring_annulus.py,test_confidence_config.py; extended rings / orchestrator / body-technique / PNG testsaddopts = ["-m", "not integration"]default; CIrun-tests.ymlupdatedTesting
-m ""or-m integrationto opt in; the two new operator-verified sidecars (N1447064164,W1444747627) are covered by the existing parameterized regression testtest_nav_technique_ring_annulus.py(16 tests covering planted-offset recovery, multi-planet Z-buffer paint, infeasibility, at-edge, hard-zero confidence, diagnostics, empty-inputValueError, and the four_upsample_factorvalidation paths);test_confidence_config.py(20 tests covering bundled-YAML round-trip, malformed input, andload_technique_tuning); rings / orchestrator / body-technique / PNG test files extended to cover the planet-specific annulus gate, the composite-per-planet emission, the outermost-preservation pass + duplicate-key replace, the model-glob normalization, and the adaptive PNG stretch.N1447064164(km/px=2108) andW1444747627(km/px=23067): the rings model now emits a single compositeRING_ANNULUSper planet, the reliability gate accepts it, andRingAnnulusNavruns.Potential Impacts
RingAnnulusNavis added tonav.nav_technique.__all__;RING_ANNULUS_MAX_RADIAL_PXandRING_ANNULUS_KMPP_THRESHOLDare removed fromnav.nav_model.nav_model_rings.__all__(operators tuning these values now editconfig_510_techniques.yaml);LIMB_MIN_ARC_PX,TERMINATOR_MIN_ARC_PX,SPURIOUS_*, and_AT_EDGE_TOLERANCE_PXare deleted from per-technique modules and replaced by per-techniquetuning:YAML blocks read viacls.tuning.pytestno longer collects integration tests) affects CI matrix steps that relied on the old default. CI'srun-tests.ymlandscripts/run-all-checks.share updated to reflect the new convention.np.median+np.median(np.abs(...))per summary PNG (negligible against the existing FFT pyramid); composite annulus rendering is cheaper than per-ring annulus rendering since the OR-and-max is one pass over each ext-FOV array.config_510_techniques.yaml(operator runbook inPHASE6_LIBRARY_SEED.md).Checklist
ruff check,ruff format)mypypasses (274 source files clean)-Wbuild green;pymarkdown scanclean)CHANGES.mdin this repoNotes
phase_06_review/(the sevenCRITIQUE_*.mdfiles for the per-phase definition of done) is intentionally excluded from this PR per request.PHASE6_LIBRARY_SEED.md.🤖 Generated with Claude Code