feat: improve star/ring navigation and correlation accuracy#91
Merged
rfrenchseti merged 14 commits intomainfrom Apr 21, 2026
Merged
feat: improve star/ring navigation and correlation accuracy#91rfrenchseti merged 14 commits intomainfrom
rfrenchseti merged 14 commits intomainfrom
Conversation
WalkthroughSplit/added focused config files and logging controls; introduced MAIN/IMAGE loggers and per-image logs; implemented ring-shadow masking and ring-plane occlusion for stars; added photometric gating in star refinement; changed masked NCC to return numerator and propagate max-offset constraints through pyramid navigation; updated UI, docs, and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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. 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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/user_guide_navigation.rst (1)
338-379:⚠️ Potential issue | 🟡 MinorFix stale config filename references after config split.
Line 338 and Line 379 point readers to
config_01_general.yaml, but the documented keys areoffset.*andstars.*. Those sections should reference the corresponding split config files to prevent misconfiguration.Proposed doc fix
-The following configuration options in ``config_01_general.yaml`` control the behavior of ``correlate_all``: +The following configuration options in ``config_02_offset.yaml`` control the behavior of ``correlate_all``: @@ -Example from ``config_01_general.yaml``: +Example from ``config_03_stars.yaml``:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/user_guide_navigation.rst` around lines 338 - 379, The docs incorrectly reference config_01_general.yaml; update those references to the actual split config files that now contain the documented keys (replace the occurrences at the offsets section and the ring occlusion section). Specifically, change the mention of config_01_general.yaml where you document offset.* and general.log_level_nav_correlate_all to the config file that contains the offset/logging keys (the file that defines offset.correlation_fft_upsample_factor, offset.star_refinement_*, and general.log_level_nav_correlate_all), and change the mention where you document stars.ring_occlusion_radii_km to the config file that contains the stars.* keys (the file that defines stars.ring_occlusion_radii_km). Ensure the referenced filenames match the repo's split config filenames and update any example snippet headers to the same filenames.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nav/config_files/config_01_general.yaml`:
- Line 13: The hardcoded truetype_font_dir value should be replaced with a
configurable, platform-aware default: read from an environment variable (e.g.,
TRUETYPE_FONT_DIR) if set, otherwise detect platform (process.platform /
os.platform) and choose a sensible default per OS (Linux:
/usr/share/fonts/truetype, macOS: /Library/Fonts or ~/Library/Fonts, Windows:
C:\Windows\Fonts), or call a font-discovery helper if available; update the
config key truetype_font_dir to use that resolved value and add
documentation/comments in config_01_general.yaml explaining how to override on
non-Linux systems and suggesting a font-discovery library as an alternative.
In `@src/nav/config_files/config_05_rings.yaml`:
- Line 14: The YAML key curvature_to_reduce_features in config_05_rings.yaml
uses the numeric literal 1.5707963267948966; add a short inline comment after
that value stating it equals π/2 radians (90 degrees) — e.g., append "# pi/2
radians (90°)" to clarify intent for future maintainers.
- Around line 29-70: The file contains large commented-out configuration blocks
(e.g., parameter_font, parameter_overlay_start, parameter_overlay_step,
parameter_overlay_line_len_thickness, parameter_overlay_box_size and font
entries) that clutter the active config; either delete these legacy/commented
blocks if they are no longer needed, or move them into a separate
example/documentation file (or a dedicated comments/example section) and
reference it, ensuring all references to the identifiers like parameter_font,
parameter_overlay_start, parameter_overlay_step,
parameter_overlay_line_len_thickness, and parameter_overlay_box_size are
preserved in the example file for future use.
In `@src/nav/config_files/config_06_titan.yaml`:
- Around line 1-2: Update the titan configuration entry to document units for
atmosphere_height: modify the titan block (the key "titan" and its
"atmosphere_height" field) to include an inline comment indicating the expected
units (e.g., "# km") so maintainers immediately know the measurement units.
In `@src/nav/config_files/config_07_bootstrap.yaml`:
- Around line 12-20: Add inline YAML comments next to the angle and resolution
keys (max_phase_angle, max_incidence_angle, max_emission_angle, lon_resolution,
lat_resolution, max_subsolar_dist) showing the equivalent values in degrees
(e.g., "# 135 degrees", "# 70 degrees", "# 0.5 degrees", "# 45 degrees") so the
numeric radian values remain unchanged but are immediately human-readable; use
standard YAML comment syntax (#) and place comments on the same lines as the
existing entries for quick reference.
In `@src/nav/config/logger.py`:
- Around line 78-85: The setup_logging() routine is adding stream and file
handlers to MAIN_LOGGER every time it's called (via calls to
pdslogger.stream_handler and pdslogger.file_handler), causing duplicate logs;
make setup_logging() idempotent by first checking/clearing existing handlers on
MAIN_LOGGER (or detecting existing stream/file handlers) before calling
MAIN_LOGGER.add_handler, or return early if already configured, and then add the
handlers only when absent so repeated calls don't append duplicates.
- Around line 46-51: The current code assumes `level` is a string and calls
`level.upper()` which can raise unclear errors or allow invalid values; in the
function that computes the log level (the block returning level.upper()),
validate that `level` is either None or an instance of str, raise a TypeError if
it is not, then normalize with level = level.strip() and ensure the uppercased
value is one of the allowed log levels (e.g.,
"DEBUG","INFO","WARNING","ERROR","CRITICAL"); if it is not, raise a ValueError
with a clear message before returning the normalized string. Ensure you update
the code paths that call this logic (the function that reads from
`arguments`/`config.general`) to surface these exceptions.
In `@src/nav/nav_model/nav_model_stars.py`:
- Around line 585-586: The code accesses external YAML-backed config attributes
directly which can raise AttributeError if keys are missing; change reads of
self._stars_config.ring_occlusion_enabled and self._stars_config.ring_occlusion
(and the other instance around lines ~899–900) to use dictionary-style .get(...)
with safe defaults (False for ring_occlusion_enabled and {} for ring_occlusion),
and ensure _mark_conflicts_obj still receives the computed rings_can_conflict
boolean; additionally, add a small validation at the public API boundary to
raise a clear ValueError/TypeError if the config object is not a dict-like
structure expected by nav_model_stars.
- Around line 902-916: The loop validating annulus entries (iterating raw ->
planet_key, pairs -> pair and appending to validated) must first ensure each
pair is exactly a 2-item sequence of numeric values and that both numbers are
finite before coercing to float; update the checks in that block (referencing
raw, planet_key, pairs, pair, validated, and the ring_occlusion_radii_km
validation) to reject non-sequences, sequences with length != 2, non-numeric
elements, or NaN/inf and raise a clear ValueError describing the planet_key and
offending value instead of relying on len(pair) or float() throwing incidental
errors.
In `@src/nav/nav_technique/nav_technique_correlate_all.py`:
- Around line 238-248: The code currently assumes opt_metadata returned from
psfmodel.PSF.find_position() contains keys reduced_chi2, peak_snr, x_err, y_err,
scale, noise_rms; wrap the extraction of opt_v, opt_u, opt_metadata and the
subsequent access to opt_metadata['reduced_chi2'], ['peak_snr'], ['x_err'],
['y_err'], ['scale'], ['noise_rms'] in a try/except that catches KeyError and
TypeError, and on error raise a ValueError that includes the offending key name
(or type issue) and instead of letting the exception propagate mark the star as
rejected with the flag 'REFINEMENT METADATA' so the pipeline continues
gracefully; reference psfmodel.PSF.find_position(), opt_metadata, opt_v/opt_u
and the rejection flag name when implementing the change.
In `@src/nav/navigate_image_files.py`:
- Around line 75-76: The per-image logging is recording raw CLI args
(sys.argv[1:]) from navigate_image_files.py into log_run_environment, exposing
secrets; fix by sanitizing before logging: update log_run_environment (in
src/nav/support/misc.py) to sanitize/redact sensitive patterns (signed URLs,
tokens, keys, passwords, emails) from the incoming command_list or add a helper
like sanitize_cli_args and call it inside log_run_environment, then log the
sanitized ' '.join(command_list) instead; alternatively, change the call site in
navigate_image_files.py to pass a sanitized copy of sys.argv[1:] to
log_run_environment rather than the raw list.
In `@src/nav/support/correlate.py`:
- Around line 573-574: Add a brief clarifying comment next to the prior_weight
assignment in the correlate function where prior_shift/ prior_at_scale are set
(the lines using prior_weight_final and prior_at_scale) explaining that when
coarser_prior_fullres is None (i.e., at the coarsest level) prior_at_scale will
be None and thus prior_weight is intentionally set to 0.0 so the coarsest-level
quality is unpenalised; reference the symbols prior_weight, prior_weight_final,
prior_at_scale, prior_shift so reviewers can easily locate the exact assignment
to annotate.
- Line 101: Extract the magic epsilon into a module-level constant (e.g. add
_NCC_EPS = 1e-12 near the top with other constants) and replace local uses of
eps in this file (the occurrences at the current function in correlate.py and
the other uses referenced at lines ~112 and ~126) with _NCC_EPS; update any
local variable assignments like eps = 1e-12 to reference _NCC_EPS so all places
use the single constant name.
In `@src/nav/support/misc.py`:
- Around line 150-160: Expand the get_local_host_name() docstring to be
testable: describe the function purpose, the return type and value (str), the
caching behavior via _LOCAL_HOST_NAME_CACHE, and the fallback behavior when
socket.getfqdn() raises an exception (returns the literal 'LOCAL HOST NAME
FAILED'); also mention side effects (updates the module-level
_LOCAL_HOST_NAME_CACHE) so a black-box test can assert return values and
subsequent cached returns from get_local_host_name().
- Around line 125-144: Update the docstring for current_git_version to include a
"Raises:" section and clarify that the function does not propagate exceptions
(it catches all exceptions internally and returns a string), and document the
return contract (returns the git describe string or the sentinel 'GIT DESCRIBE
FAILED'); reference the function current_git_version and the module-level cache
_GIT_VERSION_CACHE so readers know the result is cached for the process
lifetime.
In `@src/nav/ui/manual_nav_dialog.py`:
- Around line 143-170: The displayed image currently uses the NCC surface
`shifted` but the global peak is computed from `shifted` via
`np.argmax(shifted)`, which can pick an arbitrary plateau; instead compute the
peak from the numerator surface returned by `masked_ncc()` (the numerator
variable) and use that to set `peak_row`, `peak_col`, `peak_dv`, and `peak_du`
(the same places where `np.argmax(shifted)` is used), while keeping `shifted`
for plotting the NCC image; apply the same change to the other peak computation
site around lines 342-343 so the green peak marker always uses the
numerator-based argmax.
---
Outside diff comments:
In `@docs/user_guide_navigation.rst`:
- Around line 338-379: The docs incorrectly reference config_01_general.yaml;
update those references to the actual split config files that now contain the
documented keys (replace the occurrences at the offsets section and the ring
occlusion section). Specifically, change the mention of config_01_general.yaml
where you document offset.* and general.log_level_nav_correlate_all to the
config file that contains the offset/logging keys (the file that defines
offset.correlation_fft_upsample_factor, offset.star_refinement_*, and
general.log_level_nav_correlate_all), and change the mention where you document
stars.ring_occlusion_radii_km to the config file that contains the stars.* keys
(the file that defines stars.ring_occlusion_radii_km). Ensure the referenced
filenames match the repo's split config filenames and update any example snippet
headers to the same filenames.
🪄 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: 3f26ac5b-e724-4627-bea4-f6e4ef67ee6d
📒 Files selected for processing (39)
docs/conf.pydocs/developer_guide.rstdocs/developer_guide_nav_technique_correlate.rstdocs/developer_guide_navigation_models_rings.rstdocs/introduction_configuration.rstdocs/user_guide_navigation.rstsrc/main/nav_offset.pysrc/nav/config/__init__.pysrc/nav/config/logger.pysrc/nav/config_files/config_01_general.yamlsrc/nav/config_files/config_01_settings.yamlsrc/nav/config_files/config_02_offset.yamlsrc/nav/config_files/config_03_stars.yamlsrc/nav/config_files/config_04_bodies.yamlsrc/nav/config_files/config_05_rings.yamlsrc/nav/config_files/config_06_titan.yamlsrc/nav/config_files/config_07_bootstrap.yamlsrc/nav/config_files/config_10_satellites.yamlsrc/nav/config_files/config_20_jupiter_rings.yamlsrc/nav/config_files/config_21_saturn_rings.yamlsrc/nav/config_files/config_22_uranus_rings.yamlsrc/nav/config_files/config_23_neptune_rings.yamlsrc/nav/config_files/config_30_inst_coiss.yamlsrc/nav/config_files/config_31_inst_gossi.yamlsrc/nav/config_files/config_32_inst_nhlorri.yamlsrc/nav/config_files/config_33_inst_vgiss.yamlsrc/nav/config_files/config_90_backplanes.yamlsrc/nav/config_files/config_95_pds4.yamlsrc/nav/nav_master/nav_master.pysrc/nav/nav_model/nav_model_rings.pysrc/nav/nav_model/nav_model_stars.pysrc/nav/nav_model/rings/ring_feature.pysrc/nav/nav_technique/nav_technique_correlate_all.pysrc/nav/navigate_image_files.pysrc/nav/support/correlate.pysrc/nav/support/misc.pysrc/nav/ui/manual_nav_dialog.pytests/nav/nav_model/test_nav_model_rings.pytests/nav/support/test_correlate.py
💤 Files with no reviewable changes (4)
- src/nav/config_files/config_10_satellites.yaml
- src/nav/config_files/config_90_backplanes.yaml
- src/nav/config_files/config_95_pds4.yaml
- src/nav/config_files/config_01_settings.yaml
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #91 +/- ##
==========================================
+ Coverage 40.76% 42.36% +1.60%
==========================================
Files 70 70
Lines 6229 6446 +217
Branches 875 916 +41
==========================================
+ Hits 2539 2731 +192
- Misses 3500 3509 +9
- Partials 190 206 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 (2)
src/nav/ui/manual_nav_dialog.py (1)
768-784:⚠️ Potential issue | 🟠 MajorClamp the auto offset to the same slice-safe bounds as dragging.
max_offset_vuallows peaks at the extended-FOV margin, but the drag path clamps tomargin - 1“to keep slices valid.” Auto can now set an endpoint offset and immediately refresh the overlay with it.Suggested fix
) dv, du = float(res['offset'][0]), float(res['offset'][1]) + dv = float( + np.clip( + dv, + -self._obs.extfov_margin_v + 1, + self._obs.extfov_margin_v - 1, + ) + ) + du = float( + np.clip( + du, + -self._obs.extfov_margin_u + 1, + self._obs.extfov_margin_u - 1, + ) + ) self._dv, self._du = dv, du🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/ui/manual_nav_dialog.py` around lines 768 - 784, The auto-computed offsets from navigate_with_pyramid_kpeaks are allowed up to self._obs.extfov_margin_vu but must be clamped to the same slice-safe bounds used by dragging (i.e., margin - 1) before applying them; after retrieving res['offset'] into dv, du, clamp each to the range [-(self._obs.extfov_margin_vu - 1), +(self._obs.extfov_margin_vu - 1)] and then assign to self._dv/self._du, update the spin boxes (_spin_dv/_spin_du) and call _refresh_overlay() so the overlay is rendered with the slice-safe, clamped offsets.src/nav/support/correlate.py (1)
425-434:⚠️ Potential issue | 🔴 CriticalKeep the no-candidate result schema compatible with pyramid logging.
With
max_offset_vu, a level can legitimately have no eligible peaks. The fallback omitspeak_val, but the pyramid loop logsres_lvl["peak_val"], causing aKeyErrorinstead of returning a spurious/low-quality result.Suggested fix
if not candidates: # TODO When no candidates are found, the function returns cov: np.diag([1e6, 1e6]) # and quality: -np.inf. Downstream code might not check for -np.inf quality and could # treat this as a valid result. Consider returning None or raising an exception instead. return { 'offset': (0.0, 0.0), 'cov': np.diag([1e6, 1e6]), 'sigma_xy': (1e3, 1e3), 'quality': -np.inf, + 'peak_val': -np.inf, + 'rc': (-1, -1), }Also applies to: 568-591
🤖 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 425 - 434, The fallback branch that returns when `candidates` is empty is missing the `peak_val` field, causing pyramid logging to KeyError on `res_lvl["peak_val"]`; update that return dict to include a `peak_val` key (set to a sensible sentinel such as 0.0 or -np.inf to match the low-quality signal) alongside the existing `offset`, `cov`, `sigma_xy`, and `quality` values, and make the same change to the analogous fallback block later in the file (the second empty-candidates branch around the 568-591 region) so both branches provide a consistent result schema for downstream code that reads `res_lvl["peak_val"]`.
♻️ Duplicate comments (2)
src/nav/nav_model/nav_model_stars.py (2)
633-634:⚠️ Potential issue | 🟠 MajorDefault optional ring-occlusion config keys before use.
This still directly reads YAML-backed attributes, so configs missing
ring_occlusion_enabledorring_occlusion_radii_kmcan raiseAttributeErrorinstead of using safe defaults.Suggested fix
- rings_can_conflict = self._stars_config.ring_occlusion_enabled + rings_can_conflict = bool(self._stars_config.get('ring_occlusion_enabled', False)) self._mark_conflicts_obj(star, rings_can_conflict) @@ - raw: dict[str, list[list[float]]] = self._stars_config.ring_occlusion_radii_km or {} + raw_obj = self._stars_config.get('ring_occlusion_radii_km', {}) or {} + if not isinstance(raw_obj, dict): + raise ValueError( + 'stars.ring_occlusion_radii_km must be a mapping of planet name ' + 'to annulus lists' + ) + raw: dict[str, object] = raw_objAs per coding guidelines, "Validate inputs at the public API boundary of the library. Raise clear
ValueErrororTypeErrorexceptions for invalid arguments."Also applies to: 947-948
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/nav_model/nav_model_stars.py` around lines 633 - 634, The code reads YAML-backed attributes directly (e.g., self._stars_config.ring_occlusion_enabled and ring_occlusion_radii_km) which can raise AttributeError; update accesses to use safe defaults via getattr(self._stars_config, "ring_occlusion_enabled", False) and getattr(..., "ring_occlusion_radii_km", []) (or equivalent) before calling _mark_conflicts_obj; additionally validate types/values (raise TypeError if not bool/list or ValueError for invalid radii) at the public API boundary where _stars_config is set/loaded so callers get clear errors; apply the same changes for the other occurrence that uses ring_occlusion_* around _mark_conflicts_obj.
950-953:⚠️ Potential issue | 🟠 MajorValidate each planet’s annulus list before iterating it.
pairsstill comes from YAML and may be a scalar/string/mapping; iterating it can produce incidental errors or misleading per-character validation. Require a list/tuple of annulus pairs first.Suggested fix
for planet_key, pairs in raw.items(): + if not isinstance(pairs, (list, tuple)): + raise ValueError( + f'ring_occlusion_radii_km: expected a list of annulus pairs for ' + f'{planet_key!r}, got {type(pairs).__name__}: {pairs!r}' + ) validated: list[tuple[float, float]] = [] for pair in pairs:As per coding guidelines, "NEVER trust external input (function arguments from callers, file contents, environment variables, data from remote URLs)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/nav_model/nav_model_stars.py` around lines 950 - 953, The loop over raw items currently assumes pairs is iterable and a list of annulus pairs; before iterating, ensure pairs is actually a list/tuple (not a scalar/string/mapping) and skip or raise a clear validation error if not. In the block iterating raw.items(), add a type check for the variable pairs (from the planet_key iteration) and only proceed to build validated and call _ring_occlusion_annulus_pair(pair, planet_key) when pairs is an instance of list or tuple; for non-list inputs, log or raise a descriptive error mentioning planet_key so malformed YAML entries are not iterated per-character.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/nav_offset.py`:
- Around line 321-329: navigate_image_files(...) returns (success_bool,
metadata_dict) but the call is used directly in an if, so the tuple is always
truthy; change the call to unpack the result such as "success, metadata =
navigate_image_files(...)" and then use "if success:" to gate incrementing
NUM_FILES_PROCESSED and any downstream logic that should only run on successful
navigation; keep references to navigate_image_files, NUM_FILES_PROCESSED and
metadata to ensure the metadata_dict is preserved for later use.
In `@src/nav/config/logger.py`:
- Around line 53-58: The code uses getattr(arguments, attr_name, None) to read
an argparse.Namespace; replace that with vars(arguments).get(attr_name) so the
Namespace is accessed via its dict form; specifically update the branch that
sets level from the local variable "arguments" (the expression using
getattr(arguments, attr_name, None)) to use vars(arguments).get(attr_name) and
keep the existing fallback to config.general via getattr(config.general,
attr_name, None).
In `@src/nav/support/correlate.py`:
- Around line 554-561: The scaling of max_offset_vu to max_offset_at_scale
currently uses integer floor division which can drop valid offsets at coarse
pyramid levels; update the computation in correlate.py where max_offset_at_scale
is computed (referencing max_offset_vu and s) to use ceiling scaling (e.g.,
math.ceil(max_offset_vu[0] / s) and math.ceil(max_offset_vu[1] / s)) while still
enforcing a minimum of 1 for each component, and add the necessary import for
math if not present.
In `@src/nav/support/misc.py`:
- Around line 178-200: The function log_run_environment currently logs
command_list verbatim; change it to sanitize/redact sensitive values before
logging by mapping command_list through a sanitizer (e.g., new helper
sanitize_arg or existing sanitizer utility) that masks tokens, signed URLs,
emails, API keys, passwords, and local paths (use pattern checks for “://”, long
hex/token-like strings, email regex, and home/absolute paths) and then log
'Command line: %s' with ' '.join(sanitized_list) instead of the raw
command_list; update or add unit tests for sanitize_arg and ensure the sanitizer
is used anywhere else command strings are logged.
---
Outside diff comments:
In `@src/nav/support/correlate.py`:
- Around line 425-434: The fallback branch that returns when `candidates` is
empty is missing the `peak_val` field, causing pyramid logging to KeyError on
`res_lvl["peak_val"]`; update that return dict to include a `peak_val` key (set
to a sensible sentinel such as 0.0 or -np.inf to match the low-quality signal)
alongside the existing `offset`, `cov`, `sigma_xy`, and `quality` values, and
make the same change to the analogous fallback block later in the file (the
second empty-candidates branch around the 568-591 region) so both branches
provide a consistent result schema for downstream code that reads
`res_lvl["peak_val"]`.
In `@src/nav/ui/manual_nav_dialog.py`:
- Around line 768-784: The auto-computed offsets from
navigate_with_pyramid_kpeaks are allowed up to self._obs.extfov_margin_vu but
must be clamped to the same slice-safe bounds used by dragging (i.e., margin -
1) before applying them; after retrieving res['offset'] into dv, du, clamp each
to the range [-(self._obs.extfov_margin_vu - 1), +(self._obs.extfov_margin_vu -
1)] and then assign to self._dv/self._du, update the spin boxes
(_spin_dv/_spin_du) and call _refresh_overlay() so the overlay is rendered with
the slice-safe, clamped offsets.
---
Duplicate comments:
In `@src/nav/nav_model/nav_model_stars.py`:
- Around line 633-634: The code reads YAML-backed attributes directly (e.g.,
self._stars_config.ring_occlusion_enabled and ring_occlusion_radii_km) which can
raise AttributeError; update accesses to use safe defaults via
getattr(self._stars_config, "ring_occlusion_enabled", False) and getattr(...,
"ring_occlusion_radii_km", []) (or equivalent) before calling
_mark_conflicts_obj; additionally validate types/values (raise TypeError if not
bool/list or ValueError for invalid radii) at the public API boundary where
_stars_config is set/loaded so callers get clear errors; apply the same changes
for the other occurrence that uses ring_occlusion_* around _mark_conflicts_obj.
- Around line 950-953: The loop over raw items currently assumes pairs is
iterable and a list of annulus pairs; before iterating, ensure pairs is actually
a list/tuple (not a scalar/string/mapping) and skip or raise a clear validation
error if not. In the block iterating raw.items(), add a type check for the
variable pairs (from the planet_key iteration) and only proceed to build
validated and call _ring_occlusion_annulus_pair(pair, planet_key) when pairs is
an instance of list or tuple; for non-list inputs, log or raise a descriptive
error mentioning planet_key so malformed YAML entries are not iterated
per-character.
🪄 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: 8a240f53-88eb-4ded-be22-ef1296fab7ae
📒 Files selected for processing (7)
src/main/nav_offset.pysrc/nav/config/logger.pysrc/nav/nav_model/nav_model_stars.pysrc/nav/nav_technique/nav_technique_correlate_all.pysrc/nav/support/correlate.pysrc/nav/support/misc.pysrc/nav/ui/manual_nav_dialog.py
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/nav_offset.py (1)
3-75:⚠️ Potential issue | 🟡 MinorUpdate the stale CLI usage name.
Line 3 now identifies this as
nav_offset.py, but the usage text still printsnav_main_offseton Lines 59, 67, and 75.Proposed fix
- print('Usage: nav_main_offset <dataset_name> [args]') + print('Usage: nav_offset <dataset_name> [args]') @@ - print('Usage: nav_main_offset <dataset_name> [args]') + print('Usage: nav_offset <dataset_name> [args]') @@ - print('Usage: nav_main_offset <dataset_name> [args]') + print('Usage: nav_offset <dataset_name> [args]')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/nav_offset.py` around lines 3 - 75, The CLI usage text is stale: in parse_args (function parse_args) update all printed usage strings that say "nav_main_offset" to the correct script name "nav_offset" (replace occurrences in the print calls that show usage and unknown-dataset help lines so the messages in parse_args reflect the current filename).src/nav/support/correlate.py (2)
158-202: 🛠️ Refactor suggestion | 🟠 MajorValidate
max_offset_vuat the API boundary.This is a new public parameter; a negative component silently wipes the whole surface to
-infand returns no peaks (the caller then hits the empty-candidate path). Reject invalid input explicitly.🛡️ Proposed validation
corr_v, corr_u = corr.shape work = corr.copy() if max_offset_vu is not None: max_v, max_u = max_offset_vu + if max_v < 0 or max_u < 0: + raise ValueError( + f'max_offset_vu components must be non-negative, got {max_offset_vu}' + ) rows = np.arange(corr_v)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/support/correlate.py` around lines 158 - 202, nms_topk currently accepts max_offset_vu with negative components which silently masks the whole surface; add explicit validation at the start of nms_topk to ensure max_offset_vu is either None or a tuple of two non-negative integers (or ints castable), and raise a clear ValueError (or TypeError if the types are wrong) if the tuple length is not 2 or any component is negative; update the checks around max_v, max_u (the variables derived from max_offset_vu) so that they only run after validation and include this explicit error handling in the nms_topk function signature.
426-436:⚠️ Potential issue | 🟠 MajorKeyError risk in pyramid-level logging when
max_offset_vuprunes all peaks.When
nms_topkreturns an empty list (easy to trigger now thatmax_offset_at_scalecan clamp to(1, 1)at the coarsest level, or if a caller passes a tightmax_offset_vu),navigate_single_scale_kpeaksreturns the fallback dict at Lines 430-435, which omitspeak_val(andrc). The pyramid-level debug log at Line 594 then raisesKeyError: 'peak_val', masking the real "no candidates within offset bound" failure and aborting the navigation before the final full-resolution pass can run.Either normalize the empty-candidate dict to include the keys the rest of the module reads, or make the logging robust. Normalizing is strictly better because downstream consumers (e.g.
evaluate_candidatecallers, tests) will behave consistently.🐛 Proposed fix
if not candidates: # TODO When no candidates are found, the function returns cov: np.diag([1e6, 1e6]) # and quality: -np.inf. Downstream code might not check for -np.inf quality and could # treat this as a valid result. Consider returning None or raising an exception instead. return { 'offset': (0.0, 0.0), 'cov': np.diag([1e6, 1e6]), 'sigma_xy': (1e3, 1e3), 'quality': -np.inf, + 'peak_val': 0.0, + 'rc': (0, 0), }Also applies to: 589-595
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/nav_offset.py`:
- Around line 158-194: The CLI accepts --log-level-image-console and
--log-level-image-file but setup_logging() only resolves main logger levels, so
validate the image log level args immediately after parsing (same place where
you validate --log-level-main-console and --log-level-main-file) so invalid
values error out at startup; call the same validation logic used for main levels
(or reuse the helper that translates/checks level names) to canonicalize/raise
on --log-level-image-console and --log-level-image-file before entering the
batch loop and before calling setup_logging(), referencing the CLI arg names and
setup_logging() so reviewers can find the change.
- Around line 317-320: The current MAIN_LOGGER.info call is logging full
external URLs via imagefiles.image_files -> f.image_file_url.as_posix(), which
may expose tokens; update the log to emit a stable non-sensitive identifier
(e.g., f.results_path_stub or imagefiles.results_path_stub) instead of the full
URL. Locate the MAIN_LOGGER.info call and replace the join over
f.image_file_url.as_posix() with a join over the results_path_stub property for
each image file (or a single results_path_stub from imagefiles if available) so
logs contain only the safe identifier.
---
Outside diff comments:
In `@src/main/nav_offset.py`:
- Around line 3-75: The CLI usage text is stale: in parse_args (function
parse_args) update all printed usage strings that say "nav_main_offset" to the
correct script name "nav_offset" (replace occurrences in the print calls that
show usage and unknown-dataset help lines so the messages in parse_args reflect
the current filename).
In `@src/nav/support/correlate.py`:
- Around line 158-202: nms_topk currently accepts max_offset_vu with negative
components which silently masks the whole surface; add explicit validation at
the start of nms_topk to ensure max_offset_vu is either None or a tuple of two
non-negative integers (or ints castable), and raise a clear ValueError (or
TypeError if the types are wrong) if the tuple length is not 2 or any component
is negative; update the checks around max_v, max_u (the variables derived from
max_offset_vu) so that they only run after validation and include this explicit
error handling in the nms_topk function signature.
🪄 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: 266a7ca0-fbdc-4423-9b06-d3abb65592f5
📒 Files selected for processing (2)
src/main/nav_offset.pysrc/nav/support/correlate.py
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
This branch makes a broad set of improvements to navigation accuracy and reliability across the star, ring, and correlation subsystems. The primary motivation is to fix the single-bright-star case where pyramid correlation was picking far-off spurious peaks, and to add the ring-plane star-exclusion and planet-shadow-removal features needed for ring-plane imaging sequences.
Changes
Correlation (
src/nav/support/correlate.py)masked_ncc: computes the true Pearson-r NCC surface and also returns the unnormalized covariance numerator.nms_topk: new non-maximum suppression function to return top-k peaks with configurable suppression radius and amax_offset_vuwindow.navigate_with_pyramid_kpeaks: coarse-to-fine multi-peak pyramid with prior propagation.max_offset_vuis accepted and scaled (floored to 1) at each pyramid level so every level retains at least 1-pixel tolerance.navigate_single_scale_kpeaks: single-scale k-peak correlation with sub-pixel refinement.Star correlation and refinement (
src/nav/nav_technique/nav_technique_correlate_all.py)max_offset_vu=obs.extfov_margin_vunow passed tonavigate_with_pyramid_kpeaksso per-star searches are restricted to the physically-reachable offset range.reduced_chi2,peak_snr,x_err,y_err,scale, andnoise_rmsper star whenlog_star_refinement_detailis set.reduced_chi2floor: bright stars are no longer rejected solely on a low chi2 score unlesspeak_snris also below threshold, eliminating spurious outlier rejection for well-fit bright stars.vmag_spread == 0guard: if all stars have the same magnitude the reliability list defaults to 1.0 for all stars instead of dividing by zero.Ring-plane star exclusion (
src/nav/nav_model/nav_model_stars.py)_mark_conflicts_objnow performs a ring-plane backplane check whenring_occlusion_enabledisTrue(replacing the previous unconditionalTrue).ring_occlusion_radii_kmconfig key defines per-planet annuli; pairs are validated (length >= 2, inner < outer) with clearValueErrormessages.RING: <PLANET>conflict label.Ring feature extent check (
src/nav/nav_model/rings/ring_feature.py,nav_model_rings.py)RingFeature.max_extent_radiusproperty (a + ae across all edges, raisesValueErrorwhen no edges are present).NavModelRingsperforms a fast FOV extent check before the expensive resolutions backplane: if the minimum visible ring radius exceeds every feature's maximum extent, the model exits early.Planet shadow removal (
src/nav/nav_model/nav_model_rings.py)rings.remove_planet_shadowconfig flag (defaultFalse). When set, thewhere_inside_shadowbackplane is computed once and applied to every rendered feature's model image and mask. Backplane failures are caught and logged as warnings so shadow removal does not abort model creation.Manual navigation dialog (
src/nav/ui/manual_nav_dialog.py)_CorrMapDialogpopup: displays the full 2-D NCC surface as a color-mapped Matplotlib figure with the current offset overlaid as a red cross and the global peak circled in green._show_masktoggle renders the model mask as a semi-transparent white tint over the image/model composite._MASK_OVERLAY_ALPHAand_CORR_MAP_*module-level constants replace magic literals.max_offset_vuis wired fromobs.extfov_margin_vuat the call site.Utilities and logging (
src/nav/support/misc.py,config/logger.py)current_git_version(),get_local_host_name(),log_run_environment()added tomisc.pyfor consistent run-context logging.config/logger.py.Configuration (
src/nav/config_files/)ring_occlusion_radii_km,ring_occlusion_enabled,star_refinement_*keys, andremove_planet_shadow.Documentation and tests
docs/developer_guide_nav_technique_correlate.rstcovering the correlation algorithm, pyramid strategy, peak selection, and the NCC numerator rationale.docs/developer_guide_navigation_models_rings.rstdescribing the rings navigation model domain types, filtering pipeline, and rendering contracts.docs/introduction_configuration.rstanddocs/user_guide_navigation.rstfor the reorganized YAML layout and new navigation-related options.tests/nav/support/test_correlate.py: 261 tests includingmax_offset_vumasking, pyramid prior propagation, andnms_topk.tests/nav/nav_model/test_nav_model_rings.py: ring extent test.Type of Change
Testing
All 261 pytest tests pass (
pytest -n auto). Ruff, Mypy (122 files), Sphinx (-W), and PyMarkdown all pass clean. Single-star correlation was verified manually on COISS images where the previous code picked spurious off-center peaks; the revised NCC-numerator path converges to the correct star position.Potential Impacts
masked_nccreturn type changed: now returns(ncc, numerator)instead ofnccalone. Any caller outside this repo that importedmasked_nccdirectly must unpack the tuple.navigate_with_pyramid_kpeakssignature:max_offset_vukeyword argument added; callers that did not pass it are unaffected.NavModelResult/NavTechniqueAPI.Checklist
ruff check,ruff format)mypypassesNotes
The
masked_nccreturn-type change is the only public-API break; all internal callers have been updated.Summary by CodeRabbit
Documentation
New Features
Configuration
Tests