Implement ring models #80
Conversation
WalkthroughAdds full ring support across the codebase: GUI for creating/editing simulated rings, simulated and config-driven ring navigation models, ring rendering in the simulation pipeline, snapshot persistence, unit tests for ring algorithms, configuration changes for Saturn ring data, and documentation updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/nav_create_simulated_image.py (2)
1915-1930: Consider adding visual aids for rings.The
_display_imagemethod draws visual aids (red circles for body centers, yellow crosses for stars), but rings are not visualized. Consider adding ring center markers for consistency, especially since rings havecenter_vandcenter_uproperties.Example addition after star crosses
# Draw ring centers as cyan diamonds or similar pen = QPen(QColor(0, 255, 255), 1) painter.setPen(pen) for r in self.sim_params.get('rings', []): u = int(r.get('center_u', 0)) v = int(r.get('center_v', 0)) # Draw a small diamond painter.drawLine(u, v - 4, u + 4, v) painter.drawLine(u + 4, v, u, v + 4) painter.drawLine(u, v + 4, u - 4, v) painter.drawLine(u - 4, v, u, v - 4)
2010-2056: Ring drag-move support is not implemented.The
_move_selected_bymethod handles dragging for bodies and stars but not rings. Since rings havecenter_vandcenter_uproperties like bodies, consider adding ring drag support for UI consistency.Add ring drag support
elif kind == 'star' and 0 <= idx < len(self.sim_params['stars']): # ... existing star handling ... self._updater.immediate_update() + elif kind == 'ring' and 0 <= idx < len(self.sim_params['rings']): + self.sim_params['rings'][idx]['center_v'] = float( + self.sim_params['rings'][idx].get('center_v', 0.0) + dv + ) + self.sim_params['rings'][idx]['center_u'] = float( + self.sim_params['rings'][idx].get('center_u', 0.0) + du + ) + tab_idx = self._find_tab_by_properties('ring', idx) + if tab_idx is not None: + tab_w = self._tabs.widget(tab_idx) + if tab_w is not None: + cv_spin = getattr(tab_w, 'center_v_spin', None) + cu_spin = getattr(tab_w, 'center_u_spin', None) + if cv_spin is not None: + cv_spin.setValue(self.sim_params['rings'][idx]['center_v']) + if cu_spin is not None: + cu_spin.setValue(self.sim_params['rings'][idx]['center_u']) + self._updater.immediate_update() self._last_drag_img_vu = (img_v, img_u)
🤖 Fix all issues with AI agents
In @src/nav/nav_model/nav_model_rings_base.py:
- Around line 77-79: The loop in nav_model_rings_base.py unpacks (edge_mask,
label_text, edge_type) but never uses edge_type; rename it to _edge_type in the
unpacking (for edge_mask, label_text, _edge_type in edge_info_list) to indicate
intentional unused variable and satisfy linters while preserving the tuple
structure and intent in the surrounding loop inside the relevant method/class.
In @src/nav/nav_model/nav_model_rings_simulated.py:
- Around line 116-117: The assignments to time and epoch use a redundant hasattr
check; replace "time = getattr(obs, 'sim_time', 0.0) if hasattr(obs, 'sim_time')
else 0.0" and "epoch = getattr(obs, 'sim_epoch', 0.0) if hasattr(obs,
'sim_epoch') else 0.0" with the simpler "time = getattr(obs, 'sim_time', 0.0)"
and "epoch = getattr(obs, 'sim_epoch', 0.0)" so the default handles missing
attributes for obs.
In @src/nav/nav_model/nav_model_rings.py:
- Around line 889-904: The parameter radii_mvals is declared but unused in
_render_single_edge (and similarly in _render_full_ringlet); remove radii_mvals
from the method signatures and update all call sites to stop passing that
argument, and if any tests or internal documentation reference radii_mvals
remove or update those references as well so no unused-parameter warnings
remain.
- Around line 129-131: The ValueError message constructed when epoch_str is None
uses '{planet}' without an f-string, so it prints literally; update the raise in
the block that checks epoch_str (the line raising ValueError('No epoch
configured for planet {planet}')) to use an f-string that interpolates the
planet variable (e.g., raise ValueError(f'No epoch configured for planet
{planet}')) so the actual planet name is included.
- Around line 952-954: The variable max_res returned from the call to
self._find_resolutions_by_a(obs, ring_target, a=edge_a) is unused; update the
assignment in the method (where min_res, max_res are set) to use a discard
variable (e.g., min_res, _max_res = self._find_resolutions_by_a(...)) so the
intent is clear and linter warnings are resolved.
- Around line 807-818: The parameter radii_mvals on _render_full_ringlet is
unused; remove it from the method signature and all call sites (replace calls
that pass radii_mvals with calls without that argument, e.g., remove
radii_mvals=... or the positional argument), and update any related type
hints/tests/docs; ensure _compute_edge_radii still provides the radii inside
_render_full_ringlet and run tests to verify no remaining references to
radii_mvals.
- Around line 356-373: Replace the broad "except Exception" handlers around the
utc_to_et calls with more specific exception types (e.g., except (ValueError,
TypeError) as e) for both the start_date -> start_et and end_date -> end_et
blocks so parsing errors are caught without masking other exceptions; keep the
existing self._logger.warning(...) message and continue behavior, and ensure you
reference utc_to_et, start_date, end_date, start_et, end_et, and feature_key
when making the change.
In @src/nav/nav_technique/nav_technique_correlate_all.py:
- Around line 104-107: The dead `if False` debug block that imports matplotlib
and calls plt.imshow/plt.show around combined_model.model_img should be removed
or replaced with a configurable debug flag; introduce a module-level boolean
like DEBUG_SHOW_MODEL (default False) or use the existing logger to gate
visualization so the import and plt calls only occur when enabled, or simply
delete the block if not needed; ensure references to combined_model.model_img
remain correct when moving the visualization behind the flag and avoid
unconditional matplotlib imports when the feature is disabled.
In @src/nav/sim/sim_ring.py:
- Around line 171-172: The line initializing border as (abs_diff == 0.0) in
sim_ring.py is effectively dead due to floating-point equality; remove this
exact-equality check or replace it with a documented epsilon-based comparison if
you intend to keep theoretical zero handling. Specifically, update the
initialization of the variable border (currently using abs_diff == 0.0) by
either deleting that assignment and relying on the existing neighbor sign-change
logic, or change it to an epsilon test (e.g., abs_diff <= eps) and document eps,
referring to the variables border and abs_diff and the surrounding
neighbor-based sign-change logic in the same function.
- Around line 232-244: Replace the explicit loops that search for mode==1 with
Python's next(...) pattern to match the style in nav_model_rings_simulated.py:
use inner_mode1 = next((m for m in inner_data if m.get('mode') == 1), None) and
similarly outer_mode1 = next((m for m in outer_data if m.get('mode') == 1),
None); this keeps behavior identical (returns the first matching dict or None)
while aligning the code style for inner_mode1/outer_mode1 lookups.
In @tests/nav/nav_model/test_nav_model_rings_edge_fade.py:
- Around line 258-262: The v_grid from the meshgrid unpacking (created alongside
u_grid from u_coords, v_coords) is never used; rename it to _v_grid (or _v) to
signal an intentionally unused variable in the u_grid, v_grid = np.meshgrid(...)
unpack and do the same change for the similar meshgrid at the later occurrence
(around the block currently at lines ~297-302) so linters and readers know the
variable is intentionally ignored.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
src/main/nav_create_simulated_image.pysrc/nav/config_files/config_20_saturn_rings.yamlsrc/nav/nav_master/nav_master.pysrc/nav/nav_model/__init__.pysrc/nav/nav_model/nav_model_rings.pysrc/nav/nav_model/nav_model_rings_base.pysrc/nav/nav_model/nav_model_rings_simulated.pysrc/nav/nav_technique/nav_technique_correlate_all.pysrc/nav/obs/obs_inst_sim.pysrc/nav/sim/render.pysrc/nav/sim/sim_ring.pysrc/nav/support/time.pytests/nav/nav_model/test_nav_model_rings_antialiasing.pytests/nav/nav_model/test_nav_model_rings_edge_fade.pytests/nav/nav_model/test_nav_model_rings_feature_filtering.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-23T19:46:29.815Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 68
File: src/nav/dataset/dataset_pds3_cassini_iss.py:241-241
Timestamp: 2025-12-23T19:46:29.815Z
Learning: In Python, for staticmethod methods, super() cannot be used because there is no class or instance context. If a subclass static method needs to call a parent class's static method, invoke it directly as ParentClass.method_name() rather than super().method_name(). This applies broadly to Python files in the repository; ensure the parent method is defined as a static method or accessible in the class hierarchy and that imports are correct.
Applied to files:
src/nav/sim/render.pytests/nav/nav_model/test_nav_model_rings_feature_filtering.pytests/nav/nav_model/test_nav_model_rings_edge_fade.pysrc/nav/nav_model/__init__.pysrc/nav/nav_model/nav_model_rings_base.pysrc/nav/obs/obs_inst_sim.pysrc/nav/nav_technique/nav_technique_correlate_all.pysrc/nav/nav_model/nav_model_rings_simulated.pysrc/nav/support/time.pytests/nav/nav_model/test_nav_model_rings_antialiasing.pysrc/nav/nav_model/nav_model_rings.pysrc/nav/nav_master/nav_master.pysrc/nav/sim/sim_ring.pysrc/main/nav_create_simulated_image.py
📚 Learning: 2025-11-10T22:26:37.929Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 14
File: nav/support/correlate_old.py:83-88
Timestamp: 2025-11-10T22:26:37.929Z
Learning: The file nav/support/correlate_old.py contains legacy code and should be excluded from code reviews.
Applied to files:
src/nav/nav_technique/nav_technique_correlate_all.py
📚 Learning: 2025-11-14T03:01:15.014Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 37
File: nav/nav_model/nav_model_body.py:251-271
Timestamp: 2025-11-14T03:01:15.014Z
Learning: In the simulated body navigation model (`NavModelBody._create_simulated_model` in nav/nav_model/nav_model_body.py), the model intentionally does NOT include `sim_offset_v/u` from the observation. This deliberate misalignment between the simulated observation image (which includes offsets) and the navigation model (which uses base centers) is designed to test the navigation correlation process's ability to recover the offset, mimicking real-world positional uncertainty.
Applied to files:
src/nav/nav_model/nav_model_rings_simulated.pysrc/nav/nav_master/nav_master.py
🧬 Code graph analysis (8)
src/nav/sim/render.py (1)
src/nav/sim/sim_ring.py (1)
render_ring(198-322)
tests/nav/nav_model/test_nav_model_rings_feature_filtering.py (2)
src/nav/nav_model/nav_model_rings.py (3)
_load_ring_features(325-413)_validate_mode_data(415-470)_get_base_radius(472-491)src/nav/support/time.py (1)
utc_to_et(56-68)
src/nav/nav_model/__init__.py (1)
src/nav/nav_model/nav_model_rings_simulated.py (1)
NavModelRingsSimulated(22-255)
src/nav/obs/obs_inst_sim.py (1)
src/nav/sim/render.py (1)
render_combined_model(553-581)
src/nav/nav_technique/nav_technique_correlate_all.py (2)
src/nav/nav_master/nav_master.py (1)
combined_model(161-163)src/nav/nav_model/nav_model.py (1)
model_img(94-96)
tests/nav/nav_model/test_nav_model_rings_antialiasing.py (1)
src/nav/nav_model/nav_model_rings.py (1)
_compute_antialiasing(620-660)
src/nav/nav_model/nav_model_rings.py (5)
src/nav/annotation/annotation.py (1)
Annotation(11-106)src/nav/annotation/annotation_text_info.py (1)
AnnotationTextInfo(48-334)src/nav/config/config.py (2)
Config(9-231)rings(192-196)src/nav/support/time.py (2)
now_dt(17-24)utc_to_et(56-68)src/nav/nav_model/nav_model_rings_base.py (2)
NavModelRingsBase(29-207)_create_edge_annotations(36-207)
src/main/nav_create_simulated_image.py (1)
src/nav/obs/obs_snapshot.py (1)
closest_planet(302-303)
🪛 Ruff (0.14.10)
tests/nav/nav_model/test_nav_model_rings_feature_filtering.py
16-16: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
tests/nav/nav_model/test_nav_model_rings_edge_fade.py
16-16: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
35-35: Missing return type annotation for private function _calculate_fade_integral_shade_above
(ANN202)
81-81: Missing return type annotation for private function _calculate_fade_integral_shade_below
(ANN202)
260-260: Unpacked variable v_grid is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
299-299: Unpacked variable v_grid is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
src/nav/nav_model/__init__.py
5-5: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
src/nav/nav_model/nav_model_rings_base.py
77-77: Loop control variable edge_type not used within loop body
Rename unused edge_type to _edge_type
(B007)
src/nav/nav_model/nav_model_rings_simulated.py
83-83: Boolean-typed positional argument in function definition
(FBT001)
84-84: Boolean-typed positional argument in function definition
(FBT001)
85-85: Boolean-typed positional argument in function definition
(FBT001)
106-106: Logging statement uses f-string
(G004)
tests/nav/nav_model/test_nav_model_rings_antialiasing.py
16-16: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
src/nav/nav_model/nav_model_rings.py
97-97: Boolean-typed positional argument in function definition
(FBT001)
98-98: Boolean-typed positional argument in function definition
(FBT001)
99-99: Boolean-typed positional argument in function definition
(FBT001)
122-122: Logging statement uses f-string
(G004)
130-130: Avoid specifying long messages outside the exception class
(TRY003)
135-135: Avoid specifying long messages outside the exception class
(TRY003)
139-139: Avoid specifying long messages outside the exception class
(TRY003)
141-142: Logging statement uses f-string
(G004)
161-161: Logging statement uses f-string
(G004)
176-176: Logging statement uses f-string
(G004)
294-294: Boolean positional value in function call
(FBT003)
306-306: Boolean positional value in function call
(FBT003)
358-358: Do not catch blind exception: Exception
(BLE001)
360-361: Logging statement uses f-string
(G004)
369-369: Do not catch blind exception: Exception
(BLE001)
371-372: Logging statement uses f-string
(G004)
387-387: Logging statement uses f-string
(G004)
395-395: Logging statement uses f-string
(G004)
435-435: Logging statement uses f-string
(G004)
441-441: Logging statement uses f-string
(G004)
447-447: Logging statement uses f-string
(G004)
467-468: Logging statement uses f-string
(G004)
611-611: Boolean positional value in function call
(FBT003)
705-706: Logging statement uses f-string
(G004)
710-711: Logging statement uses f-string
(G004)
816-816: Unused method argument: radii_mvals
(ARG002)
840-840: Logging statement uses f-string
(G004)
851-851: Logging statement uses f-string
(G004)
855-856: Logging statement uses f-string
(G004)
861-861: Boolean positional value in function call
(FBT003)
897-897: Unused method argument: radii_mvals
(ARG002)
929-930: Logging statement uses f-string
(G004)
938-939: Logging statement uses f-string
(G004)
953-953: Unpacked variable max_res is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
956-956: Logging statement uses f-string
(G004)
964-965: Logging statement uses f-string
(G004)
src/nav/sim/sim_ring.py
56-56: Comment contains ambiguous ν (GREEK SMALL LETTER NU). Did you mean v (LATIN SMALL LETTER V)?
(RUF003)
97-97: Comment contains ambiguous ν (GREEK SMALL LETTER NU). Did you mean v (LATIN SMALL LETTER V)?
(RUF003)
src/main/nav_create_simulated_image.py
1606-1606: Unused lambda argument: checked
(ARG005)
1746-1746: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
🔇 Additional comments (26)
src/nav/config_files/config_20_saturn_rings.yaml (1)
22-28: LGTM - Well-structured ring configuration.The new
rings.ring_features.SATURNstructure with epoch, feature_width, and min_fade_width_multiplier parameters is cleanly organized. The epoch format matches the expected UTC format for time conversions.src/main/nav_create_simulated_image.py (2)
121-125: LGTM - sim_params extended for ring support.The additions of
time,epoch, andringsfields properly extend the simulation parameters to support ring models.
1481-1523: Ring tab follows established patterns well.The
_build_ring_tabmethod mirrors the body and star tab implementations with appropriate form fields for ring parameters. The center position spinboxes are stored as widget properties for potential drag updates.src/nav/sim/render.py (2)
525-529: LGTM - Ring rendering correctly integrated.Rings are rendered before bodies (as they are farther away in the scene), and the time/epoch parameters are properly extracted from sim_params and passed to
render_ring. The loop structure is straightforward and consistent with the rendering pipeline.
540-549: LGTM - Metadata correctly extended with rings.The
ringsfield is appropriately added to the metadata dictionary, maintaining consistency with how bodies and stars are tracked.src/nav/nav_model/__init__.py (1)
5-5: LGTM - NavModelRingsSimulated correctly exported.The import and
__all__entry properly exposeNavModelRingsSimulatedas part of the public API. Thenoqa: F401directive is appropriate since this is a re-export pattern (the import is used via__all__).src/nav/support/time.py (1)
56-68: LGTM - Clean inverse of et_to_utc.The
utc_to_etfunction correctly chains the julian library's conversions (UTC → TAI → TDB/ET) as the inverse ofet_to_utc. The explicitfloat()cast ensures a consistent return type, and the docstring clearly documents the expected input format.tests/nav/nav_model/test_nav_model_rings_feature_filtering.py (4)
13-29: Comprehensive mock for ring model testing.The
MockObservationclass provides the minimal interface needed for testingNavModelRings. Consider adding the return type annotation for__init__for consistency with type hints elsewhere.Optional: Add return type annotation
- def __init__(self, midtime=None): + def __init__(self, midtime=None) -> None:
39-68: LGTM - Good baseline test for features without date ranges.This test establishes that features without
start_date/end_dateare always included, which is the expected default behavior.
348-449: Excellent comprehensive radius filtering test.This test covers multiple scenarios in a single test: features in range, below range, above range, and exactly at boundaries. The assertions clearly verify which features should be included or excluded based on radius constraints.
518-582: Good coverage for single-edge gap features.Testing gaps with only inner or outer edges ensures the filtering logic handles partial feature definitions correctly. This is important for real-world configurations where some features may have only one defined edge.
src/nav/obs/obs_inst_sim.py (1)
86-96: LGTM! Clean integration of ring metadata into the snapshot.The new
sim_time,sim_epoch, andsim_ringsattributes are correctly extracted with safe defaults. The updated comment accurately reflects the new rendering order (rings, stars, then bodies).tests/nav/nav_model/test_nav_model_rings_antialiasing.py (2)
110-141: Good test coverage that explicitly documents implementation behavior.The test correctly matches and documents the implementation's clipping behavior where
shade > 1.0is set to0.0(as noted in the implementation comment about potential legacy bug). This ensures the tests remain consistent with the implementation while preserving awareness of the quirk.
13-32: Well-structured test scaffolding.The
MockObservationprovides the minimal interface required byNavModelRings, and the pytest fixture cleanly instantiates the model for each test.src/nav/nav_master/nav_master.py (2)
15-19: LGTM! Type hierarchy correctly updated for ring model polymorphism.The imports and type annotations properly use
NavModelRingsBaseas the base type, enabling bothNavModelRingsandNavModelRingsSimulatedto be handled uniformly.Also applies to: 77-77, 139-139
281-322: Well-structured ring model computation with proper branching for simulated vs. real observations.The method correctly:
- Handles early exit when no closest planet exists
- Branches appropriately between simulated (using
NavModelRingsSimulated) and real (usingNavModelRings) observations- Validates configuration availability before proceeding
- Provides clear logging for debugging
tests/nav/nav_model/test_nav_model_rings_edge_fade.py (2)
35-124: Thorough helper functions that accurately replicate implementation logic.The
_calculate_fade_integral_shade_aboveand_calculate_fade_integral_shade_belowhelpers correctly implement the case-based integration logic, enabling precise expected-value calculations for test assertions.
235-250: Good edge case coverage for narrow fade widths.The test correctly verifies that
_compute_edge_fadereturnsNonewhenradius_width_km < min_radius_width_km, preventing invalid fade computations.src/nav/nav_model/nav_model_rings_base.py (2)
102-135: Effective tangent direction estimation using weighted variance.The approach of using inverse-distance-weighted variance to determine whether the edge runs more vertically or horizontally is a practical heuristic for label placement. The discretization to pure vertical/horizontal directions simplifies subsequent label offset calculations.
191-205: LGTM! Clean annotation creation with proper avoid mask.The
maximum_filteron the model mask creates an appropriate buffer zone for text placement, and theAnnotationconstruction includes all necessary configuration for rendering.src/nav/nav_model/nav_model_rings_simulated.py (2)
168-255: Clean edge annotation creation with proper FOV coordinate handling.The method correctly:
- Distinguishes between inner/outer edges and RINGLET/GAP feature types for labeling
- Computes edge masks using the simulated border computation
- Properly embeds data-coordinate masks into extended FOV space
- Delegates to the unified base class method for actual annotation creation
143-150: No issue found—negative values are prevented by clipping inrender_ring.The
render_ringfunction already clips output to[0.0, 1.0]for both RINGLET and GAP feature types (vianp.clip(..., 0.0, 1.0)at line 321 and 324 insrc/nav/sim/sim_ring.py). For GAP, even though values are subtracted, the result is clipped to a minimum of 0.0, never negative. Therefore,modelwill always remain in the[0, 1]range when merging withnp.minimum, and no downstream handling of negative values is needed.Likely an incorrect or invalid review comment.
src/nav/sim/sim_ring.py (2)
14-63: Correct implementation of elliptical orbit radius computation.The function properly:
- Computes the angle from center to pixel using
atan2- Handles time-dependent precession of the longitude of pericenter
- Clamps eccentricity to prevent mathematical issues
- Applies the standard elliptical orbit equation r = a(1 - e²) / (1 + e·cos(ν))
297-322: Correct anti-aliasing implementation for ring edges.The fade computations properly handle:
inner_fade: 0 well inside the inner edge, transitioning to 1 at/outside the inner edgeouter_fade: 1 at/inside the outer edge, transitioning to 0 outside the outer edgering_coverage = min(inner_fade, outer_fade): correctly identifies the ring region between edgesThe 1-pixel anti-aliasing width provides smooth edge transitions.
src/nav/nav_model/nav_model_rings.py (2)
654-658: Verify the intentional preservation of legacy edge-case behavior.The comment acknowledges that setting
shade > 1.0to0.0(instead of clipping to1.0) "seems like a bug," yet the code implements this behavior for compatibility. This could cause visible discontinuities at edges where the shade value briefly exceeds 1.0.Consider adding a TODO or tracking issue to revisit this, or verify with tests that this behavior is actually required for correlation accuracy.
38-60: LGTM - Well-structured class with comprehensive documentation.The class design properly inherits from
NavModelRingsBaseand the constructor correctly delegates to the parent class. The type hints are appropriate and the integration withoops.Observationis well-defined.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #80 +/- ##
==========================================
- Coverage 35.90% 33.73% -2.18%
==========================================
Files 58 61 +3
Lines 4726 5529 +803
Branches 597 715 +118
==========================================
+ Hits 1697 1865 +168
- Misses 2929 3559 +630
- Partials 100 105 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In @src/nav/nav_model/nav_model_rings_base.py:
- Around line 75-77: The loop unpacks (edge_mask, label_text, edge_type) from
edge_info_list but never uses edge_type; rename it to _edge_type in the
unpacking to satisfy convention and static analysis (e.g., change "for
edge_mask, label_text, edge_type in edge_info_list:" to use "_edge_type").
Update any other occurrences in the same loop header or nearby unpacking of the
same pattern (e.g., other for-loops over edge_info_list) to consistently use
_edge_type.
- Around line 135-154: The tangent angle computation (variables tangent_angle
and tangent_angle_deg and their normalization) in nav_model_rings_base.py is
dead code and should be removed: delete the math.atan2 and math.degrees calls
and the tangent_angle_deg = tangent_angle_deg % 360 line as well as the
surrounding comments that describe angle normalization, leaving only the
normalized direction components (du_norm, dv_norm) and the existing
abs_du/abs_dv-based label placement logic (refer to tangent_angle,
tangent_angle_deg and abs_du/abs_dv to locate the code); ensure no other code
references tangent_angle or tangent_angle_deg before removing.
In @src/nav/nav_model/nav_model_rings_simulated.py:
- Around line 116-117: Replace redundant hasattr+getattr patterns by using
getattr with a default directly: change "time = getattr(obs, 'sim_time', 0.0) if
hasattr(obs, 'sim_time') else 0.0" and "epoch = getattr(obs, 'sim_epoch', 0.0)
if hasattr(obs, 'sim_epoch') else 0.0" to simply "time = getattr(obs,
'sim_time', 0.0)" and "epoch = getattr(obs, 'sim_epoch', 0.0)"; apply the same
simplification for the other occurrences of sim_time/sim_epoch access later in
this module (the similar patterns around the second block near the later lines)
so all uses rely only on getattr(obs, '<attr>', default).
In @src/nav/nav_model/nav_model_rings.py:
- Around line 888-889: The parameter radii_mvals is unused in the method
signature (same as in _render_full_ringlet); either remove radii_mvals from the
signature or rename it to _radii_mvals to mark it as intentionally unused, and
update any callers or tests that pass this argument accordingly; also update the
function's docstring/type hints to reflect the change so there are no lingering
references to radii_mvals.
- Around line 807-809: The parameter radii_mvals in _render_full_ringlet is
unused; either remove it from the function signature and update any callers that
pass that argument, or if you need to keep the parameter for API compatibility,
rename it to _radii_mvals to mark it intentionally unused; update the function
signature in _render_full_ringlet and all call sites accordingly to avoid
unused-parameter warnings.
- Line 944: The tuple unpacking that currently does "min_res, max_res =
self._find_resolutions_by_a(obs, ring_target, a=edge_a)" leaves max_res unused;
rename the unused variable to "_max_res" (i.e., "min_res, _max_res =
self._find_resolutions_by_a(...)") in the method where this call occurs so it
follows Python convention for unused values and avoids linter warnings while
keeping the call to self._find_resolutions_by_a intact.
- Around line 120-121: The ValueError raised when epoch_str is None uses a
literal '{planet}' placeholder instead of interpolating the variable; update the
raise to interpolate planet (e.g., replace the string in the epoch_str check
with an f-string like f'No epoch configured for planet {planet}' or use
.format(planet=planet)) so the actual planet name appears in the exception
raised by the epoch_str check.
In @src/nav/sim/sim_ring.py:
- Around line 248-249: Replace the silent early return when inner_mode1 or
outer_mode1 is missing with a logged warning so missing mode-1 configuration is
visible: in the block that currently checks "if inner_mode1 is None or
outer_mode1 is None: return" (within the function that renders the ring in
sim_ring.py), call the module's logger (or warnings.warn) to emit a clear
warning including which of inner_mode1/outer_mode1 is None and any identifying
ring/context info, then return as before.
- Around line 60-63: The code silently clamps eccentricity when computing e = ae
/ a in sim_ring.py; instead of silently setting e = 0.99 when e >= 1.0, emit a
warning that includes the computed eccentricity and the involved values (ae, a)
so users notice invalid/config issues; update the block around e, the clamp, and
r calculation to call the project's logger or warnings.warn (e.g., include
variable names e, ae, a, true_anomaly in the message) before setting e = 0.99.
- Around line 68-106: compute_edge_radius_mode1 duplicates the elliptical radius
math; refactor it to compute the angle/true anomaly parameters it currently
calculates and then call compute_edge_radius_at_angle to perform the shared
ellipse formula. Specifically, have compute_edge_radius_mode1 compute
days_since_epoch and current_long_peri (using long_peri, rate_peri, epoch,
time), derive the true angle to pass as the angle argument, compute/forward a
and ae (or derive e there if mode1 currently does), and return the value from
compute_edge_radius_at_angle(angle=..., a=a, ae=ae, long_peri=long_peri,
rate_peri=rate_peri, epoch=epoch, time=time); remove the duplicate math (e,
clamp, and r calculation) from compute_edge_radius_mode1 so only
compute_edge_radius_at_angle contains the ellipse equation.
In @tests/nav/nav_model/test_nav_model_rings_edge_fade.py:
- Around line 258-262: The v_grid produced by np.meshgrid is unused and should
be renamed or dropped to follow convention; change the meshgrid call that
creates u_grid and v_grid to either u_grid, _v_grid = np.meshgrid(u_coords,
v_coords) or remove v_grid entirely and update any other similar meshgrid usages
(there is a second occurrence referenced around the later block where u_grid is
used to compute radii) so that only u_grid is kept and the unused variable is
prefixed with an underscore (_v_grid) or eliminated.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
src/main/nav_create_simulated_image.pysrc/nav/nav_master/nav_master.pysrc/nav/nav_model/nav_model_rings.pysrc/nav/nav_model/nav_model_rings_base.pysrc/nav/nav_model/nav_model_rings_simulated.pysrc/nav/sim/sim_ring.pytests/nav/nav_model/test_nav_model_rings_edge_fade.pytests/nav/nav_model/test_nav_model_rings_feature_filtering.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-23T19:46:29.815Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 68
File: src/nav/dataset/dataset_pds3_cassini_iss.py:241-241
Timestamp: 2025-12-23T19:46:29.815Z
Learning: In Python, for staticmethod methods, super() cannot be used because there is no class or instance context. If a subclass static method needs to call a parent class's static method, invoke it directly as ParentClass.method_name() rather than super().method_name(). This applies broadly to Python files in the repository; ensure the parent method is defined as a static method or accessible in the class hierarchy and that imports are correct.
Applied to files:
tests/nav/nav_model/test_nav_model_rings_feature_filtering.pysrc/nav/nav_model/nav_model_rings_base.pytests/nav/nav_model/test_nav_model_rings_edge_fade.pysrc/nav/nav_model/nav_model_rings.pysrc/nav/nav_model/nav_model_rings_simulated.pysrc/nav/sim/sim_ring.pysrc/nav/nav_master/nav_master.pysrc/main/nav_create_simulated_image.py
📚 Learning: 2025-11-14T03:01:15.014Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 37
File: nav/nav_model/nav_model_body.py:251-271
Timestamp: 2025-11-14T03:01:15.014Z
Learning: In the simulated body navigation model (`NavModelBody._create_simulated_model` in nav/nav_model/nav_model_body.py), the model intentionally does NOT include `sim_offset_v/u` from the observation. This deliberate misalignment between the simulated observation image (which includes offsets) and the navigation model (which uses base centers) is designed to test the navigation correlation process's ability to recover the offset, mimicking real-world positional uncertainty.
Applied to files:
src/nav/nav_model/nav_model_rings_simulated.pysrc/nav/nav_master/nav_master.py
🧬 Code graph analysis (6)
tests/nav/nav_model/test_nav_model_rings_feature_filtering.py (4)
src/nav/nav_model/nav_model_rings.py (3)
_load_ring_features(316-404)_validate_mode_data(406-461)_get_base_radius(463-482)src/nav/support/time.py (1)
utc_to_et(56-68)src/nav/obs/obs_snapshot.py (2)
closest_planet(302-303)extdata_shape_vu(274-275)src/nav/nav_master/nav_master.py (1)
obs(106-108)
src/nav/nav_model/nav_model_rings_base.py (3)
src/nav/annotation/annotation.py (2)
Annotation(11-106)avoid_mask(85-87)src/nav/annotation/annotations.py (2)
Annotations(13-183)add_annotations(26-54)src/nav/annotation/annotation_text_info.py (6)
AnnotationTextInfo(48-334)text_loc(82-84)ref_vu(87-89)font(97-99)font_size(102-104)color(92-94)
tests/nav/nav_model/test_nav_model_rings_edge_fade.py (1)
src/nav/nav_model/nav_model_rings.py (1)
_compute_edge_fade(653-796)
src/nav/nav_model/nav_model_rings.py (3)
src/nav/config/config.py (1)
rings(192-196)src/nav/support/time.py (2)
now_dt(17-24)utc_to_et(56-68)src/nav/nav_model/nav_model_rings_base.py (2)
NavModelRingsBase(27-207)_create_edge_annotations(34-207)
src/nav/nav_model/nav_model_rings_simulated.py (2)
src/nav/sim/sim_ring.py (2)
render_ring(200-324)compute_border_atop_simulated(109-197)src/nav/nav_model/nav_model_rings_base.py (1)
_create_edge_annotations(34-207)
src/main/nav_create_simulated_image.py (2)
src/nav/obs/obs_snapshot.py (1)
closest_planet(302-303)src/nav/annotation/annotation_text_info.py (1)
text(77-79)
🪛 Ruff (0.14.10)
tests/nav/nav_model/test_nav_model_rings_feature_filtering.py
16-16: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
src/nav/nav_model/nav_model_rings_base.py
75-75: Loop control variable edge_type not used within loop body
Rename unused edge_type to _edge_type
(B007)
tests/nav/nav_model/test_nav_model_rings_edge_fade.py
16-16: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
35-35: Missing return type annotation for private function _calculate_fade_integral_shade_above
(ANN202)
81-81: Missing return type annotation for private function _calculate_fade_integral_shade_below
(ANN202)
260-260: Unpacked variable v_grid is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
299-299: Unpacked variable v_grid is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
src/nav/nav_model/nav_model_rings.py
88-88: Boolean-typed positional argument in function definition
(FBT001)
89-89: Boolean-typed positional argument in function definition
(FBT001)
90-90: Boolean-typed positional argument in function definition
(FBT001)
113-113: Logging statement uses f-string
(G004)
121-121: Avoid specifying long messages outside the exception class
(TRY003)
126-126: Avoid specifying long messages outside the exception class
(TRY003)
130-130: Avoid specifying long messages outside the exception class
(TRY003)
132-133: Logging statement uses f-string
(G004)
152-152: Logging statement uses f-string
(G004)
167-167: Logging statement uses f-string
(G004)
285-285: Boolean positional value in function call
(FBT003)
297-297: Boolean positional value in function call
(FBT003)
349-349: Do not catch blind exception: Exception
(BLE001)
351-352: Logging statement uses f-string
(G004)
360-360: Do not catch blind exception: Exception
(BLE001)
362-363: Logging statement uses f-string
(G004)
378-378: Logging statement uses f-string
(G004)
386-386: Logging statement uses f-string
(G004)
426-426: Logging statement uses f-string
(G004)
432-432: Logging statement uses f-string
(G004)
438-438: Logging statement uses f-string
(G004)
458-459: Logging statement uses f-string
(G004)
602-602: Boolean positional value in function call
(FBT003)
696-697: Logging statement uses f-string
(G004)
701-702: Logging statement uses f-string
(G004)
807-807: Unused method argument: radii_mvals
(ARG002)
831-831: Logging statement uses f-string
(G004)
842-842: Logging statement uses f-string
(G004)
846-847: Logging statement uses f-string
(G004)
852-852: Boolean positional value in function call
(FBT003)
888-888: Unused method argument: radii_mvals
(ARG002)
920-921: Logging statement uses f-string
(G004)
929-930: Logging statement uses f-string
(G004)
944-944: Unpacked variable max_res is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
947-947: Logging statement uses f-string
(G004)
955-956: Logging statement uses f-string
(G004)
src/nav/nav_model/nav_model_rings_simulated.py
83-83: Boolean-typed positional argument in function definition
(FBT001)
84-84: Boolean-typed positional argument in function definition
(FBT001)
85-85: Boolean-typed positional argument in function definition
(FBT001)
106-106: Logging statement uses f-string
(G004)
src/nav/sim/sim_ring.py
58-58: Comment contains ambiguous ν (GREEK SMALL LETTER NU). Did you mean v (LATIN SMALL LETTER V)?
(RUF003)
99-99: Comment contains ambiguous ν (GREEK SMALL LETTER NU). Did you mean v (LATIN SMALL LETTER V)?
(RUF003)
src/main/nav_create_simulated_image.py
1606-1606: Unused lambda argument: checked
(ARG005)
1746-1746: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test nav (ubuntu-latest, 3.10)
- GitHub Check: Test nav (ubuntu-latest, 3.13)
- GitHub Check: Test nav (ubuntu-latest, 3.12)
- GitHub Check: Test nav (ubuntu-latest, 3.11)
🔇 Additional comments (26)
src/main/nav_create_simulated_image.py (12)
121-125: LGTM!The addition of
time,epoch, andringsfields follows the existing data model structure. Note thatclosest_planetis not initialized here but will be set by_on_closest_planetor_load_parameters, which is acceptable since the UI defaults to empty.
258-270: LGTM!The editable combo box provides flexibility for custom planet names while offering common presets. The initialization logic correctly handles both predefined values and custom text.
592-598: LGTM!The handler correctly normalizes input (strips whitespace, uppercases) and stores
Nonefor empty values, which aligns with the pattern expected by downstream ring model code.
773-785: LGTM!Ring option follows the same pattern as Body and Star, maintaining UI consistency.
789-801: LGTM!The uniqueness check now correctly includes rings alongside bodies and stars, preventing name collisions across all model types.
886-922: LGTM!The ring tab creation follows the established pattern for bodies and stars. The default ring structure with
inner_dataandouter_datamode 1 entries provides a reasonable starting configuration.
980-982: LGTM!Ring deletion handling is correctly integrated following the existing body/star pattern.
1058-1066: LGTM!Ring tabs are properly integrated into the tab ordering (Bodies → Rings → Stars), with consistent alphabetical sorting by name.
1481-1611: Ring tab implementation follows existing patterns well.A few observations:
Missing
rmsUI control: Theinner_dataandouter_datainclude anrmsfield (defaulted to 1.0), but there's no spin box to edit it. If this is intentional (users must edit JSON directly), consider adding a comment. Otherwise, UI controls should be added.No visual aids for rings: The
_display_imagemethod (lines 1915-1930) draws visual aids for bodies (red ellipses) and stars (yellow crosses) but not for rings. Consider adding visual indicators for ring centers.No drag support for rings: The
_select_model_atand_move_selected_bymethods don't handle rings, so users cannot drag-move ring centers. If this is intentional due to the different nature of ring positioning, that's fine; otherwise, consider adding support.
1759-1793: LGTM!The mode 1 handlers correctly find or create the mode 1 entry in
inner_data/outer_dataand ensurermsis always present. The find-or-create pattern properly mutates the existing dict when found, which works becausenext()returns a reference to the actual list item.
1746-1757: LGTM!The ring field and name handlers follow the same pattern as their body and star counterparts, maintaining code consistency.
2118-2140: LGTM!The load functionality correctly handles the new fields (
time,epoch,closest_planet,rings) and properly updates the UI, including the closest planet combo box with both predefined and custom value support.src/nav/nav_model/nav_model_rings_base.py (1)
1-52: LGTM - Well-structured base class with sensible defaults.The class design with configuration fallbacks from
rings_configtobodies_configis clean and provides good flexibility. The method signature with typed parameters is clear.src/nav/sim/sim_ring.py (1)
109-197: LGTM - Border detection logic is correct.The sign-change detection between neighbors for edge identification is a standard and effective approach. The use of both vertical and horizontal neighbor checks ensures comprehensive edge detection.
src/nav/nav_model/nav_model_rings_simulated.py (2)
144-150: Verify gap handling with negative model values.When
render_ringprocesses a GAP on a zero-initializedring_img, it subtractsring_coverage, producing negative values. Usingnp.minimumthen setsmodelto these negative values. If downstream processing expectsmodelvalues in[0, 1], this could cause issues.Is negative model imagery for gaps intentional (to represent "absence" of material), or should gaps be handled differently in simulated models?
30-46: LGTM - Clean constructor with proper initialization.The constructor properly copies
sim_ringsto avoid mutation issues and delegates to the base class correctly.src/nav/nav_model/nav_model_rings.py (2)
645-648: Preserved bug: values > 1.0 are zeroed instead of clipped.The comment acknowledges this might be a bug in the original code but preserves it for compatibility. Consider whether this behavior is actually correct: if
shade > 1.0indicates the pixel is fully outside the anti-aliasing region, zeroing might be intentional. If not, clipping to 1.0 would be more standard.Please verify whether zeroing values
> 1.0is the intended behavior or should beshade[shade > 1.0] = 1.0.
29-51: LGTM - Clean class structure inheriting from base.The class properly extends
NavModelRingsBaseand the constructor is straightforward.tests/nav/nav_model/test_nav_model_rings_feature_filtering.py (3)
13-30: LGTM - Well-designed mock observation for testing.The
MockObservationprovides the necessary interface for testing ring model methods without requiring full observation infrastructure.
39-69: LGTM - Good test coverage for date-based filtering.The test suite comprehensively covers all date filtering scenarios: no dates, within range, before range, after range, and partial date constraints.
357-458: Good boundary condition testing for radius filtering.The test correctly validates that features at exact boundary values (
min_radiusandmax_radius) are included, while features outside are excluded. The assertions clearly verify the expected filtering behavior.tests/nav/nav_model/test_nav_model_rings_edge_fade.py (2)
35-78: Good approach: helper functions mirror implementation for verification.The helper functions
_calculate_fade_integral_shade_aboveand_calculate_fade_integral_shade_belowintentionally replicate the implementation logic to verify correctness. This is an effective testing pattern.
253-289: Thorough conflict detection test.The test correctly verifies that when a nearby feature (110.0 km) is within the fade width (20.0 km from edge at 100.0 km), the width is adjusted to
abs(110.0 - 100.0) / 2 = 5.0 km. The per-pixel verification using the adjusted width ensures correct behavior.src/nav/nav_master/nav_master.py (3)
15-19: LGTM - Proper imports for new ring model classes.The imports correctly add
NavModelRingsSimulatedfrom the nav_model package andNavModelRingsBasefor the updated type annotations.
77-77: Type annotation updated correctly.The
_ring_modelstype is properly updated tolist[NavModelRingsBase]to accommodate bothNavModelRingsandNavModelRingsSimulatedinstances.
266-322: LGTM - Well-structured ring model computation with proper branching.The
compute_ring_modelsmethod correctly:
- Checks prerequisites (closest planet, configuration)
- Branches between simulated rings (
NavModelRingsSimulated) and configured rings (NavModelRings)- Logs informative messages for skipped scenarios
- Properly initializes and registers the created model
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 (1)
src/main/nav_create_simulated_image.py (1)
1941-1956: Consider adding visual aids for rings.The
_display_imagemethod draws visual aids for bodies (red circles at centers) and stars (yellow crosses), but not for rings. If rings have configurable center points (center_v, center_u), a visual marker could be helpful for positioning them in the preview.
🤖 Fix all issues with AI agents
In @src/main/nav_create_simulated_image.py:
- Around line 1629-1633: The lambda connected to delete_btn.clicked currently
declares an unused parameter as checked=False which triggers static-analysis
noise; rename it to _ or _checked (e.g., lambda _=False, i=idx:
self._delete_tab_by_index('ring', i)) to indicate the argument is intentionally
unused while keeping the existing behavior for delete_btn, QPushButton, and the
call to self._delete_tab_by_index('ring', i).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/main/nav_create_simulated_image.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-23T19:46:29.815Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 68
File: src/nav/dataset/dataset_pds3_cassini_iss.py:241-241
Timestamp: 2025-12-23T19:46:29.815Z
Learning: In Python, for staticmethod methods, super() cannot be used because there is no class or instance context. If a subclass static method needs to call a parent class's static method, invoke it directly as ParentClass.method_name() rather than super().method_name(). This applies broadly to Python files in the repository; ensure the parent method is defined as a static method or accessible in the class hierarchy and that imports are correct.
Applied to files:
src/main/nav_create_simulated_image.py
🧬 Code graph analysis (1)
src/main/nav_create_simulated_image.py (1)
src/nav/obs/obs_snapshot.py (1)
closest_planet(302-303)
🪛 Ruff (0.14.10)
src/main/nav_create_simulated_image.py
1632-1632: Unused lambda argument: checked
(ARG005)
1772-1772: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test nav (ubuntu-latest, 3.10)
- GitHub Check: Test nav (ubuntu-latest, 3.12)
- GitHub Check: Test nav (ubuntu-latest, 3.11)
- GitHub Check: Test nav (ubuntu-latest, 3.13)
🔇 Additional comments (10)
src/main/nav_create_simulated_image.py (10)
111-126: LGTM!The data model extension for
time,epoch, andringsis consistent with existing patterns. Using.get()forclosest_planetaccess throughout ensures backward compatibility with JSON files that don't have this field.
258-288: LGTM!The closest planet combo with editable support provides flexibility, and the time/epoch controls have appropriate ranges and tooltips for TDB seconds.
610-624: LGTM!The handlers follow established patterns, and the uppercase normalization for closest planet ensures consistency with how planet names are typically represented.
799-813: LGTM!The ring option in the add dialog follows the existing pattern for bodies and stars.
815-849: LGTM!Good extension of the uniqueness check to include rings, ensuring names are unique across all entity types.
912-948: LGTM!The ring tab creation follows established patterns with sensible defaults (outer radius > inner radius, RINGLET type).
1006-1008: LGTM!Ring deletion handling follows the established pattern for bodies and stars.
1053-1092: LGTM!Ring tab management integrates well with the existing tab rebuild logic. The ordering (General → Bodies → Rings → Stars → "+") is clear and consistent.
1772-1819: LGTM!The ring field handlers follow established patterns. The mode 1 find-or-create logic correctly handles cases where the mode entry doesn't exist yet, and the
Anytype annotation is intentional for handling multiple value types (consistent with_on_star_fieldat line 1654).
2144-2169: LGTM!The save/load implementation correctly handles the new fields with backward-compatible defaults via
.get(). The closest planet combo update logic properly handles both predefined and custom values.
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 Fix all issues with AI agents
In `@docs/api_reference/api_dataset.rst`:
- Around line 4-8: Remove the inconsistent `:noindex:` Sphinx directive from the
automodule block for nav.dataset.dataset; open the automodule for symbol
nav.dataset.dataset and delete the `:noindex:` line so the module follows the
same indexing behavior as the other dataset modules (dataset_pds3, dataset_pds4,
etc.).
In `@docs/user_guide_configuration.rst`:
- Around line 16-29: The docs list of standard config files is missing the
config_95_pds4.yaml entry; update the user_guide_configuration section to add a
bullet for ``config_95_pds4.yaml`` describing its purpose (e.g., PDS4 metadata
and export settings for generated products, overrides for PDS4 label templates
and mapping of internal fields to PDS4 keys), and place it in the alphabetical
order with the other config entries so readers see all files in
``src/nav/config_files/``.
In `@docs/user_guide_simulated_images.rst`:
- Around line 147-148: Documentation for the parameter axis3 is ambiguous:
clarify that axis3 (float) has a GUI default of 80.0 but when omitted
programmatically it is computed as the minimum of axis1 and axis2; update the
axis3 description to explicitly state "GUI default: 80.0; if not provided
programmatically, defaults to min(axis1, axis2)" and mention axis1 and axis2 by
name so readers know the computed behavior.
In `@src/main/nav_create_simulated_image.py`:
- Around line 2372-2374: Replace the unconditional "assert False, f'Unknown
kind: {kind}'" with an explicit "raise AssertionError(...)" to match other
instances in this file: when the unknown branch for variable kind is hit inside
the block that sets self._last_drag_img_vu = (img_v, img_u), raise an
AssertionError containing the same message (e.g., f"Unknown kind: {kind}")
instead of using assert False so the error behavior is consistent and not
optimized away.
- Around line 1059-1061: Replace the use of "assert False, f'Unknown kind:
{widget_kind}'" in the else branch handling widget_kind with an explicit
exception throw so it isn't removed in optimized mode; change it to "raise
AssertionError(f'Unknown kind: {widget_kind}')" (or "raise AssertionError()" if
you prefer no message) so the unknown widget_kind always raises at runtime.
- Around line 2286-2295: The loop unpacks an unused variable named range_val;
change the for-loop to use a throwaway name (for example, "for _, kind, idx,
mask in objects:") so the intent is clear while keeping the existing
objects.sort(key=lambda x: x[0]) sorting; update the loop header in the block
that selects self._selected_model_key and tab handling accordingly.
- Around line 1008-1010: Replace the unreachable assert statement in the else
branch that currently reads "assert False, f'Unknown kind: {kind}'" with an
explicit exception raise: use raise AssertionError with the same message (e.g.,
raise AssertionError(f"Unknown kind: {kind}")) so the code always fails
consistently regardless of Python optimization; locate the else branch checking
the variable kind and update it accordingly.
- Around line 2453-2461: The code treats an absent/empty closest_planet by
setting _closest_planet_combo.setCurrentIndex(0), but index 0 is 'MERCURY' so
the UI incorrectly shows Mercury; to fix, add an explicit empty entry as the
first option in the combo box during UI setup (in _setup_ui) so index 0
represents "no selection", and keep the existing logic in
nav_create_simulated_image.py that sets index 0 for empty closest_planet; also
update the inline comment near the setCurrentIndex(0) call to reflect that index
0 is now the empty/fallback entry rather than "MERCURY".
In `@src/nav/config_files/config_01_settings.yaml`:
- Around line 160-168: Remove the vestigial dictionary-style comment entries
left near the font-size settings: delete the commented line "# 512: 18" under
the ring label block (near label_font_size) and the analogous commented line in
the body label block so only the active label_font_size setting remains; locate
by the keys label_font_size and label_font in both the ring and body label
sections to update them.
In `@src/nav/nav_model/nav_model_body_simulated.py`:
- Around line 36-37: The docstring claim that anti_aliasing is "always set to
the max" contradicts the implementation which hardcodes anti_aliasing=1; update
the implementation in the constructor or factory that sets anti_aliasing (look
for the anti_aliasing argument at the model creation site where anti_aliasing=1
is passed) to honor the intended behavior—either set it to a true max value
(e.g., 4 or 8) or forward the incoming anti_aliasing parameter instead of
hardcoding 1—or alternatively change the docstring text to accurately state that
anti_aliasing is set to 1 (no supersampling); choose one approach and make the
code and docstring consistent.
- Around line 27-35: The docstring lists crater and name parameters that are not
forwarded to create_simulated_body; extract the keys name, crater_fill,
crater_min_radius, crater_max_radius, crater_power_law_exponent, and
crater_relief_scale from sim_params and pass them as arguments/kwargs when
calling create_simulated_body (the call around the create_simulated_body(...)
invocation currently missing these fields), or if crater support is
intentionally omitted, remove those keys from the docstring; prefer forwarding:
read each key from sim_params with sensible defaults and include them in the
create_simulated_body call so the simulated body uses the documented crater
settings.
In `@src/nav/nav_model/nav_model_combined.py`:
- Around line 66-67: Remove the leftover debug plotting code by deleting the
commented lines "plt.figure(); plt.imshow(model_img); plt.title('After
normalization')" and "plt.show()" in the nav_model_combined module (the block
referencing model_img/plt), leaving no commented-out matplotlib debug code in
the function where the image normalization occurs.
In `@src/nav/nav_model/nav_model_rings_simulated.py`:
- Around line 87-97: The parameters always_create_model and never_create_model
in _create_model are declared but intentionally unused for API consistency; make
that explicit by either renaming them with an underscore prefix
(_always_create_model, _never_create_model) or adding a short inline comment in
the _create_model function body noting they are intentionally unused (e.g., "#
intentionally unused for API compatibility") so linters/readers understand this
is deliberate; keep the signature unchanged otherwise to preserve compatibility
with NavModelRings._create_model.
In `@src/nav/nav_model/nav_model_rings.py`:
- Around line 607-647: The _compute_antialiasing method in NavModelRings is a
byte-for-byte duplicate of NavModelRingsBase._compute_antialiasing; remove the
entire override from the NavModelRings class so the class inherits the
implementation from NavModelRingsBase, ensuring no other code depends on a
specialized NavModelRings._compute_antialiasing before deleting.
In `@src/nav/sim/render.py`:
- Line 669: The loop unpacks a first value named range_val that is never used;
update the for loop that iterates over render_items to rename that first
unpacked variable to a conventional unused-name (e.g., _range_val or _) so the
intent is clear and linters won’t flag it; locate the for statement that
currently reads "for range_val, item_type, item_params, orig_idx in
render_items:" and change the first identifier only, leaving item_type,
item_params, and orig_idx untouched.
- Around line 685-696: The gap rendering is inverted: after calling render_ring
with temp_bg = np.ones(...) you compute gap_coverage = 1.0 - temp_bg and then
assign gap_coverage into img, which makes gaps bright. Instead, write the
already-rendered temp_bg values into img for GAP rings (use temp_bg[ring_mask]
or equivalent) so that gap_model=1.0 -> temp_bg=0.0 (dark) and gap_model=0.0 ->
temp_bg=1.0 (unchanged); update the assignment that currently uses gap_coverage
to use temp_bg and keep ring_mask_map[orig_idx] logic the same.
♻️ Duplicate comments (6)
src/nav/nav_technique/nav_technique_correlate_all.py (1)
104-107: Remove blocking debug visualization before merging.This
if Trueblock will execute every timenavigate()is called, blocking execution withplt.show()until the plot window is manually closed. This will break automated pipelines, headless execution, and any production use.Either remove this block entirely, or gate it behind a proper debug flag that defaults to
False.🐛 Proposed fix: Remove the debug block
- if True: # TODO - import matplotlib.pyplot as plt - plt.imshow(combined_model.model_img) - plt.show() result = navigate_with_pyramid_kpeaks(🔧 Alternative: Gate with a module-level debug flag
Add at module level:
_DEBUG_SHOW_MODEL = False # Set True only for local debuggingThen replace the block:
- if True: # TODO - import matplotlib.pyplot as plt - plt.imshow(combined_model.model_img) - plt.show() + if _DEBUG_SHOW_MODEL: + import matplotlib.pyplot as plt + plt.imshow(combined_model.model_img) + plt.show()src/nav/nav_model/nav_model_rings.py (4)
119-121: f-string issue has been fixed.The exception message at line 120 now correctly uses an f-string (
f'No epoch configured for planet {planet}'), addressing the previously flagged bug where the literal{planet}would have been displayed.
935-936: Unused variable correctly prefixed with underscore.The
_max_resvariable (line 935) now follows Python convention for intentionally unused unpacked values, addressing the previous review feedback.
794-872:_render_full_ringletcleaned up with unused parameter removed.The
radii_mvalsparameter has been removed from the method signature, addressing previous review feedback. The method now correctly:
- Computes edge radii from mode data (lines 819-822)
- Fills solid ringlet region (lines 844-848)
- Applies anti-aliasing at edges (lines 858-868)
- Updates the model mask appropriately (lines 871-872)
343-360: Consider narrowing exception types for date parsing.The
except Exceptionhandlers (lines 345, 356) could mask unexpected errors fromutc_to_et. Since this function wraps Julian date parsing, narrowing to(ValueError, TypeError)would catch the expected parsing errors while allowing other issues to propagate.♻️ Suggested improvement
if start_date is not None: try: start_et = utc_to_et(start_date) - except Exception as e: + except (ValueError, TypeError) as e: self._logger.warning( f'Invalid start_date "{start_date}" for feature ' f'{feature_key}: {e}') continueApply the same pattern for
end_dateat line 356.src/nav/sim/sim_ring.py (1)
95-97: Consider logging when eccentricity is clamped.The silent clamping of eccentricity to 0.99 when
e >= 1.0could mask configuration errors. This pattern appears in three locations (lines 95-97, 136-138, 192-194). A debug-level log message would help users diagnose invalid eccentricity values without being intrusive.♻️ Optional enhancement
e = ae / a if a > 0 else 0.0 if e >= 1.0: + # Note: Could add logging here if a logger is available e = 0.99 # Clamp eccentricity to valid range
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (46)
docs/api_reference.rstdocs/api_reference/api_annotation.rstdocs/api_reference/api_config.rstdocs/api_reference/api_dataset.rstdocs/api_reference/api_nav_master.rstdocs/api_reference/api_nav_model.rstdocs/api_reference/api_nav_technique.rstdocs/api_reference/api_navigate_image_files.rstdocs/api_reference/api_obs.rstdocs/api_reference/api_sim.rstdocs/api_reference/api_support.rstdocs/api_reference/api_ui.rstdocs/developer_guide_backplanes.rstdocs/developer_guide_best_practices.rstdocs/developer_guide_building_docs.rstdocs/developer_guide_class_hierarchy.rstdocs/developer_guide_configuration.rstdocs/developer_guide_extending.rstdocs/developer_guide_introduction.rstdocs/index.rstdocs/introduction.rstdocs/introduction_configuration.rstdocs/introduction_overview.rstdocs/introduction_simulated_images.rstdocs/user_guide.rstdocs/user_guide_appendix_coiss.rstdocs/user_guide_appendix_gossi.rstdocs/user_guide_appendix_nhlorri.rstdocs/user_guide_appendix_vgiss.rstdocs/user_guide_backplanes.rstdocs/user_guide_configuration.rstdocs/user_guide_navigation.rstdocs/user_guide_simulated_images.rstsrc/main/nav_create_simulated_image.pysrc/nav/config_files/config_01_settings.yamlsrc/nav/nav_master/nav_master.pysrc/nav/nav_model/nav_model_body_base.pysrc/nav/nav_model/nav_model_body_simulated.pysrc/nav/nav_model/nav_model_combined.pysrc/nav/nav_model/nav_model_rings.pysrc/nav/nav_model/nav_model_rings_base.pysrc/nav/nav_model/nav_model_rings_simulated.pysrc/nav/nav_technique/nav_technique_correlate_all.pysrc/nav/sim/render.pysrc/nav/sim/sim_ring.pytests/nav/nav_model/test_nav_model_rings_edge_fade.py
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-11-14T03:01:15.014Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 37
File: nav/nav_model/nav_model_body.py:251-271
Timestamp: 2025-11-14T03:01:15.014Z
Learning: In the simulated body navigation model (`NavModelBody._create_simulated_model` in nav/nav_model/nav_model_body.py), the model intentionally does NOT include `sim_offset_v/u` from the observation. This deliberate misalignment between the simulated observation image (which includes offsets) and the navigation model (which uses base centers) is designed to test the navigation correlation process's ability to recover the offset, mimicking real-world positional uncertainty.
Applied to files:
docs/introduction_simulated_images.rstdocs/user_guide_simulated_images.rstdocs/user_guide_navigation.rstsrc/nav/nav_master/nav_master.pysrc/nav/nav_model/nav_model_rings_simulated.pysrc/nav/nav_model/nav_model_body_simulated.py
📚 Learning: 2025-09-22T21:53:11.064Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 1
File: nav/inst/inst_newhorizons_lorri.py:48-48
Timestamp: 2025-09-22T21:53:11.064Z
Learning: The `full_fov` parameter is unique to the Galileo SSI (gossi) instrument loader in `oops.hosts.galileo.ssi.from_file()`. It should not be applied to other instrument loaders like New Horizons LORRI, as different instruments have different parameters and capabilities in their respective OOPS host implementations.
Applied to files:
docs/user_guide_appendix_nhlorri.rst
📚 Learning: 2025-12-23T19:46:29.815Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 68
File: src/nav/dataset/dataset_pds3_cassini_iss.py:241-241
Timestamp: 2025-12-23T19:46:29.815Z
Learning: In Python, for staticmethod methods, super() cannot be used because there is no class or instance context. If a subclass static method needs to call a parent class's static method, invoke it directly as ParentClass.method_name() rather than super().method_name(). This applies broadly to Python files in the repository; ensure the parent method is defined as a static method or accessible in the class hierarchy and that imports are correct.
Applied to files:
tests/nav/nav_model/test_nav_model_rings_edge_fade.pysrc/nav/nav_model/nav_model_combined.pysrc/nav/nav_model/nav_model_rings_base.pysrc/nav/nav_technique/nav_technique_correlate_all.pysrc/nav/nav_model/nav_model_rings.pysrc/nav/nav_master/nav_master.pysrc/nav/sim/render.pysrc/nav/sim/sim_ring.pysrc/nav/nav_model/nav_model_body_base.pysrc/nav/nav_model/nav_model_rings_simulated.pysrc/main/nav_create_simulated_image.pysrc/nav/nav_model/nav_model_body_simulated.py
📚 Learning: 2025-11-10T22:24:08.880Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 14
File: nav/obs/__init__.py:36-44
Timestamp: 2025-11-10T22:24:08.880Z
Learning: In nav/obs/__init__.py, the __all__ list should maintain semantic grouping rather than alphabetical ordering: generic classes (Obs, ObsSnapshot, ObsSnapshotInst) first, followed by specific instrument classes (ObsCassiniISS, etc.), then helper functions (inst_names, inst_name_to_obs_class).
Applied to files:
docs/api_reference/api_obs.rst
📚 Learning: 2025-11-24T19:48:51.676Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 48
File: main/nav_main_offset.py:167-171
Timestamp: 2025-11-24T19:48:51.676Z
Learning: In the nav_main_offset.py file, it is normal and expected for the nav_default_config.yaml file to be missing. The FileNotFoundError suppression when attempting to load this default config is intentional behavior, not an error condition that needs logging.
Applied to files:
docs/user_guide_navigation.rst
📚 Learning: 2025-09-22T21:24:11.733Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 1
File: nav/annotation/annotations.py:140-141
Timestamp: 2025-09-22T21:24:11.733Z
Learning: In nav/annotation/annotations.py, overlays are stored in extended FOV shape but are cropped to data shape before being applied to the result image. The ann_num_mask uses the full overlay shape because text placement logic works in extended FOV coordinates.
Applied to files:
src/nav/nav_model/nav_model_body_base.py
🧬 Code graph analysis (10)
tests/nav/nav_model/test_nav_model_rings_edge_fade.py (2)
src/nav/nav_model/nav_model_rings.py (2)
NavModelRings(28-964)_compute_edge_fade(649-792)src/nav/nav_model/nav_model_rings_base.py (1)
_compute_edge_fade(73-189)
src/nav/nav_model/nav_model_combined.py (3)
src/nav/nav_master/nav_master.py (1)
obs(106-108)tests/nav/nav_model/test_nav_model_rings_edge_fade.py (1)
make_extfov_zeros(21-22)src/nav/nav_model/nav_model.py (1)
model_mask(99-101)
src/nav/nav_model/nav_model_rings_base.py (5)
src/nav/annotation/annotation.py (2)
Annotation(11-106)avoid_mask(85-87)src/nav/annotation/annotations.py (2)
Annotations(13-183)add_annotations(26-54)src/nav/annotation/annotation_text_info.py (6)
AnnotationTextInfo(48-334)text_loc(82-84)ref_vu(87-89)font(97-99)font_size(102-104)color(92-94)src/nav/nav_model/nav_model.py (2)
NavModel(11-147)model_mask(99-101)src/nav/nav_model/nav_model_rings.py (4)
_compute_antialiasing(607-647)_compute_edge_fade(649-792)int_func(708-713)int_func2(750-755)
src/nav/nav_technique/nav_technique_correlate_all.py (2)
src/nav/nav_master/nav_master.py (1)
combined_model(161-163)src/nav/nav_model/nav_model.py (1)
model_img(94-96)
src/nav/nav_model/nav_model_rings.py (1)
src/nav/nav_model/nav_model_rings_base.py (5)
_create_edge_annotations(191-341)_compute_antialiasing(32-71)_compute_edge_fade(73-189)int_func(105-110)int_func2(147-152)
src/nav/nav_master/nav_master.py (4)
src/nav/nav_model/nav_model_rings_simulated.py (2)
NavModelRingsSimulated(18-229)create_model(53-85)src/nav/nav_model/nav_model_rings_base.py (1)
NavModelRingsBase(25-341)src/nav/support/nav_base.py (1)
logger(38-40)src/nav/obs/obs_snapshot.py (1)
closest_planet(302-303)
src/nav/sim/render.py (2)
src/nav/sim/sim_ring.py (1)
render_ring(274-441)src/nav/support/types.py (1)
MutableStar(21-55)
src/nav/sim/sim_ring.py (2)
tests/nav/nav_model/test_nav_model_rings_edge_fade.py (1)
ring_model(29-32)tests/nav/nav_model/test_nav_model_rings_feature_filtering.py (1)
ring_model(33-36)
src/nav/nav_model/nav_model_body_base.py (2)
src/nav/config/config.py (1)
bodies(185-189)src/nav/annotation/annotation_text_info.py (3)
font(97-99)font_size(102-104)color(92-94)
src/main/nav_create_simulated_image.py (1)
src/nav/obs/obs_snapshot.py (1)
closest_planet(302-303)
🪛 Ruff (0.14.11)
tests/nav/nav_model/test_nav_model_rings_edge_fade.py
16-16: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
35-35: Missing return type annotation for private function _calculate_fade_integral_shade_above
(ANN202)
81-81: Missing return type annotation for private function _calculate_fade_integral_shade_below
(ANN202)
src/nav/nav_model/nav_model_rings.py
87-87: Boolean-typed positional argument in function definition
(FBT001)
88-88: Boolean-typed positional argument in function definition
(FBT001)
89-89: Boolean-typed positional argument in function definition
(FBT001)
112-112: Logging statement uses f-string
(G004)
120-120: Avoid specifying long messages outside the exception class
(TRY003)
125-125: Avoid specifying long messages outside the exception class
(TRY003)
129-129: Avoid specifying long messages outside the exception class
(TRY003)
131-132: Logging statement uses f-string
(G004)
151-151: Logging statement uses f-string
(G004)
166-166: Logging statement uses f-string
(G004)
281-281: Boolean positional value in function call
(FBT003)
293-293: Boolean positional value in function call
(FBT003)
345-345: Do not catch blind exception: Exception
(BLE001)
347-348: Logging statement uses f-string
(G004)
356-356: Do not catch blind exception: Exception
(BLE001)
358-359: Logging statement uses f-string
(G004)
374-374: Logging statement uses f-string
(G004)
382-382: Logging statement uses f-string
(G004)
422-422: Logging statement uses f-string
(G004)
428-428: Logging statement uses f-string
(G004)
434-434: Logging statement uses f-string
(G004)
454-455: Logging statement uses f-string
(G004)
598-598: Boolean positional value in function call
(FBT003)
692-693: Logging statement uses f-string
(G004)
697-698: Logging statement uses f-string
(G004)
825-825: Logging statement uses f-string
(G004)
836-836: Logging statement uses f-string
(G004)
840-841: Logging statement uses f-string
(G004)
846-846: Boolean positional value in function call
(FBT003)
911-912: Logging statement uses f-string
(G004)
920-921: Logging statement uses f-string
(G004)
938-938: Logging statement uses f-string
(G004)
946-947: Logging statement uses f-string
(G004)
src/nav/sim/render.py
669-669: Loop control variable range_val not used within loop body
Rename unused range_val to _range_val
(B007)
src/nav/sim/sim_ring.py
93-93: Comment contains ambiguous ν (GREEK SMALL LETTER NU). Did you mean v (LATIN SMALL LETTER V)?
(RUF003)
134-134: Comment contains ambiguous ν (GREEK SMALL LETTER NU). Did you mean v (LATIN SMALL LETTER V)?
(RUF003)
253-253: Boolean-typed positional argument in function definition
(FBT001)
317-317: Avoid specifying long messages outside the exception class
(TRY003)
src/nav/nav_model/nav_model_rings_simulated.py
88-88: Boolean-typed positional argument in function definition
(FBT001)
88-88: Unused method argument: always_create_model
(ARG002)
89-89: Boolean-typed positional argument in function definition
(FBT001)
89-89: Unused method argument: never_create_model
(ARG002)
90-90: Boolean-typed positional argument in function definition
(FBT001)
src/main/nav_create_simulated_image.py
1009-1009: Assertion always fails, replace with pytest.fail()
(PT015)
1009-1009: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
1060-1060: Assertion always fails, replace with pytest.fail()
(PT015)
1060-1060: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
1360-1360: Unused lambda argument: checked
(ARG005)
1587-1587: Unused lambda argument: checked
(ARG005)
1765-1765: Boolean positional value in function call
(FBT003)
1767-1767: Boolean positional value in function call
(FBT003)
1777-1777: Boolean positional value in function call
(FBT003)
1779-1779: Boolean positional value in function call
(FBT003)
1789-1789: Boolean positional value in function call
(FBT003)
1791-1791: Boolean positional value in function call
(FBT003)
1801-1801: Boolean positional value in function call
(FBT003)
1803-1803: Boolean positional value in function call
(FBT003)
1808-1808: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
1866-1866: Boolean-typed positional argument in function definition
(FBT001)
1898-1898: Boolean positional value in function call
(FBT003)
1900-1900: Boolean positional value in function call
(FBT003)
1933-1933: Boolean-typed positional argument in function definition
(FBT001)
1965-1965: Boolean positional value in function call
(FBT003)
1967-1967: Boolean positional value in function call
(FBT003)
2021-2021: Boolean positional value in function call
(FBT003)
2023-2023: Boolean positional value in function call
(FBT003)
2027-2027: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
2255-2255: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
2256-2256: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
2289-2289: Loop control variable range_val not used within loop body
(B007)
2373-2373: Assertion always fails, replace with pytest.fail()
(PT015)
2373-2373: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
2520-2520: Dynamically typed expressions (typing.Any) are disallowed in state
(ANN401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test nav (ubuntu-latest, 3.13)
- GitHub Check: Test nav (ubuntu-latest, 3.12)
- GitHub Check: Test nav (ubuntu-latest, 3.10)
- GitHub Check: Test nav (ubuntu-latest, 3.11)
- GitHub Check: Lint nav
🔇 Additional comments (63)
docs/developer_guide_best_practices.rst (1)
1-3: LGTM! Heading simplification is appropriate.The simplified heading removes the redundant "Developer Guide:" prefix, making the documentation structure cleaner. The reStructuredText formatting is correct with the equals signs properly matching the 14-character title length.
docs/developer_guide_introduction.rst (1)
1-3: Verify this documentation heading change was intentional.The heading has been simplified from "Developer Guide: Introduction" to just "Introduction". While this could be part of a documentation reorganization effort, it's unrelated to the core ring models implementation in this PR.
Can you confirm this change was intentional and not an accidental edit?
src/nav/nav_model/nav_model_body_base.py (4)
45-45: Good refactor: caching the bodies configuration.Storing
self._config.bodiesin a local variable avoids repeated property access where each call triggersread_config(). This is a clean pattern for consistency and minor performance improvement.
66-133: Label placement logic looks correct.The boundary checks at lines 72, 117, and 124 properly guard against out-of-bounds access. The refactor to use
body_configfor all layout parameters (label_scan_v,label_horiz_gap,label_vert_gap,label_grid_v,label_grid_u) is consistent throughout.
135-139: LGTM!The
AnnotationTextInfoconstruction correctly usesbody_configfor font, font_size, and color parameters, aligning with the expected types shown in the relevant snippet.
141-147: LGTM!The
text_avoid_maskcreation andAnnotationconstruction correctly usebody_configforlabel_mask_enlarge,label_limb_color, andoutline_thicken. The refactor is complete and consistent.src/nav/nav_model/nav_model_body_simulated.py (1)
139-140: No action needed. The base classNavModelexplicitly supports_rangeas either a scalar value or per-pixel array through its type annotation (NDArrayFloatType | float | None) and property docstring. The scalar assignment ofnp.infinnav_model_body_simulated.pyis consistent with this design and compatible with similar model types likeNavModelStars, which also use scalar range values.docs/user_guide_appendix_vgiss.rst (1)
1-8: LGTM! Placeholder documentation is well-structured.The appendix placeholder follows proper RST formatting and provides a clear indication that content will be added later. The structure is consistent with other instrument appendices in this PR.
docs/user_guide_backplanes.rst (1)
1-3: LGTM! Title change improves clarity.The updated header "Backplane Generation" is more concise and accurately describes the document's focus on the backplane generation process and tooling.
docs/user_guide_appendix_coiss.rst (1)
1-8: LGTM! Consistent placeholder documentation.The appendix follows the same structure as other instrument appendices in this PR, maintaining consistency across the documentation set.
docs/api_reference/api_ui.rst (1)
1-7: LGTM! Standard API reference configuration.The autodoc directives are correctly configured to expose the nav.ui.manual_nav_dialog module's API, including all members and inheritance information.
docs/user_guide_appendix_nhlorri.rst (1)
1-9: LGTM! Well-formatted placeholder documentation.The appendix correctly expands the LORRI acronym and maintains consistency with other instrument appendix placeholders in this PR.
docs/api_reference/api_config.rst (1)
1-12: LGTM! Well-formed API documentation.The RST syntax is correct, and the automodule directives follow standard Sphinx conventions. This documentation addition aligns with the PR's goal of expanding API reference coverage.
docs/api_reference/api_support.rst (1)
1-52: LGTM! Comprehensive API documentation.The RST syntax is correct, and all submodules under nav.support are properly documented with consistent automodule directives. The alphabetical ordering and uniform formatting enhance readability.
docs/user_guide_appendix_gossi.rst (1)
1-8: LGTM! Appropriate documentation placeholder.The RST syntax is correct, and the note clearly indicates this is a placeholder for future Galileo SSI-specific content. This approach maintains documentation structure while signaling planned additions.
docs/developer_guide_backplanes.rst (1)
1-3: LGTM! Header simplification improves conciseness.The header change from "Developer Guide: Backplanes Architecture" to "Backplanes" is appropriate and maintains correct RST syntax. The simpler title improves readability while preserving the document's purpose.
docs/api_reference/api_annotation.rst (1)
1-17: LGTM! Clean API documentation structure.The RST syntax is correct, and the annotation modules are properly documented with consistent automodule directives. This addition enhances the API reference coverage for the annotation package.
docs/user_guide_configuration.rst (1)
106-131: Documentation is accurate - all command-line options and values verified.Verified that:
- All four CLI options (
--pds3-holdings-root,--nav-results-root,--nav-models,--nav-techniques) exist in the implementation- Model names (stars, rings, titan, body:NAME) are correct per nav_master.py docstring
- Technique names (correlate_all, manual) are both supported and implemented
- Override precedence behavior matches standard argparse CLI priority
No issues detected.
docs/developer_guide_configuration.rst (1)
1-3: LGTM!The RST header underline now correctly matches the title length.
docs/introduction_configuration.rst (2)
1-149: Comprehensive configuration documentation.This new documentation file provides clear guidance on the configuration loading order, file structure, and override mechanisms. The examples are practical and the hierarchy of precedence (standard files → user default → CLI config files → CLI options) is well explained.
109-115: The environment variable names are correctly documented and match the implementation:PDS3_HOLDINGS_DIRis used insrc/nav/dataset/dataset_pds3.py:80andNAV_RESULTS_ROOTis used insrc/nav/config/config_helper.py:73. No changes needed.docs/user_guide_simulated_images.rst (1)
1-359: Excellent user documentation for simulated images.This documentation comprehensively covers:
- GUI capabilities and usage
- JSON parameter format with all field descriptions
- Ring edge mode data structures
- Navigation model architecture for simulated rings
- Complete working example
The ring model class hierarchy explanation (lines 264-275) aligns well with the updated developer class hierarchy documentation.
docs/developer_guide_class_hierarchy.rst (3)
175-189: LGTM! Ring model class hierarchy properly documented.The new class structure correctly shows:
NavModelRingsBaseas the abstract base with shared ring functionalityNavModelRingsfor real rings from ephemeris dataNavModelRingsSimulatedfor simulated rings from JSON parametersNote that
NavModelRingsSimulatedhas an additionalsim_ringsparameter in its constructor (line 187), which appropriately reflects its need for simulation-specific ring data.
281-290: Inheritance relationships correctly updated.The class hierarchy properly reflects:
NavModel <|-- NavModelRingsBase(base class for all ring models)NavModelRingsBase <|-- NavModelRings(real rings)NavModelRingsBase <|-- NavModelRingsSimulated(simulated rings)This mirrors the existing pattern used for body models (
NavModelBodyBase→NavModelBody/NavModelBodySimulated).
329-332: LGTM! Description accurately reflects the new architecture.The updated text properly describes the ring model hierarchy with shared functionality in the base class and specialized subclasses for real vs. simulated rings.
src/main/nav_create_simulated_image.py (3)
1367-1593: LGTM - Ring tab construction is well-structured.The ring tab follows the established pattern for body/star tabs with proper widget references stored for inner/outer edge control enable/disable logic. The mode 1 parameter fields are correctly wired up to the handlers.
1866-1931: Solid implementation of mutual exclusion constraint for ring edges.The logic correctly prevents both inner and outer edges from being disabled simultaneously by re-enabling the checkbox when the user tries to disable it while the other is already disabled. Reading current UI values when re-enabling (lines 1913-1919) preserves user-entered data rather than resetting to defaults.
884-922: Well-structured ring tab creation with sensible defaults.The default ring structure includes both inner and outer edge mode 1 data with appropriate default values. The unique name and range finding ensures no collisions with existing objects.
docs/api_reference/api_sim.rst (1)
1-12: LGTM - API documentation for new simulation modules.Correctly exposes the new
nav.sim.renderandnav.sim.sim_ringmodules in the API reference, aligning with the PR's ring simulation features.docs/api_reference/api_nav_master.rst (1)
1-7: LGTM - API documentation for nav_master module.Standard Sphinx automodule directive correctly exposing the navigation master module.
docs/api_reference/api_navigate_image_files.rst (1)
1-7: LGTM - API documentation for navigate_image_files module.Standard Sphinx automodule directive correctly exposing the image file navigation module.
docs/index.rst (1)
14-15: Toctree entries are valid and reference existing files.The new toctree references
introductionanduser_guide, which map todocs/introduction.rstanddocs/user_guide.rstrespectively. Both files exist and are properly configured in the documentation structure.docs/developer_guide_building_docs.rst (1)
1-3: Title formatting is now consistent with other documentation files.The change from overlined double-bordered heading to standard underlined heading aligns with the formatting used in the other new documentation files (introduction.rst, user_guide.rst, api_reference.rst).
docs/user_guide.rst (1)
13-19: All referenced user guide files exist in thedocs/directory. No issues found.docs/api_reference/api_nav_model.rst (1)
1-52: Comprehensive API documentation for nav_model modules.The automodule entries follow standard Sphinx conventions and include all nav_model submodules, including the new ring-related modules (nav_model_rings_base, nav_model_rings, nav_model_rings_simulated) introduced in this PR. All listed modules exist in the codebase.
docs/api_reference.rst (1)
7-21: Good refactoring to use toctree-based API reference structure.The change from inline automodule blocks to a toctree-based structure improves documentation organization and maintainability. All 11 referenced API reference files exist in the
docs/api_reference/directory and are properly configured.src/nav/nav_model/nav_model_combined.py (1)
74-85: LGTM!The updated range handling correctly creates a per-pixel range array when the model provides a scalar range. Initializing with
np.infensures that unmasked pixels are treated as infinitely distant during the closest-model selection (line 96), allowing proper depth ordering when combining multiple models with per-feature masks.docs/introduction_overview.rst (1)
1-100: LGTM!The overview documentation is well-structured and provides a clear introduction to RMS-NAV. The three-phase pipeline description, installation options, and command-line tool listings are comprehensive and well-organized.
tests/nav/nav_model/test_nav_model_rings_edge_fade.py (6)
13-32: LGTM!The
MockObservationclass provides a minimal but sufficient interface for testing the edge fade computation, and thering_modelfixture is properly scoped.
35-124: LGTM!The helper functions correctly implement independent expected value calculations that mirror the production code's case-based logic. This is a good testing pattern that allows verification against an independent implementation of the same mathematical formulas.
The missing return type annotations flagged by static analysis are acceptable for private test helpers.
127-179: LGTM!Comprehensive test with a well-designed 5×5 grid covering multiple scenarios: pixels at the edge, below the edge, within the fade region, above the fade region, non-integer centers, and varying resolutions. The per-pixel assertion loop with descriptive failure messages is excellent for debugging.
235-250: LGTM!Good edge case test verifying that when
radius_width_km < min_radius_width_km, the method correctly returnsNoneto skip the feature.
253-289: LGTM!Excellent conflict detection test. The feature at 110.0 km (within the 20 km fade width from edge at 100.0 km) correctly triggers width adjustment to
abs(110.0 - 100.0) / 2 = 5.0 km, and the test verifies the adjusted fade behavior.
361-388: LGTM!Good test verifying that edge fade adds to existing model values rather than replacing them. Starting with a model filled with 0.5 and confirming the result equals
0.5 + clipped_shadeis the right approach.docs/introduction_simulated_images.rst (4)
1-100: LGTM!The documentation provides comprehensive coverage of the simulated image feature, including the GUI, JSON parameter file schema, and all top-level field descriptions. The structure is clear and well-organized.
182-228: LGTM!The ring parameters documentation is thorough, covering all required fields, edge mode data structure, and the relationship between inner/outer edges. The explanation of mode 1 as the base elliptical edge with optional higher modes for complex shapes is clear.
248-275: LGTM!The ring navigation models section accurately describes the class hierarchy (
NavModelRingsBase→NavModelRings/NavModelRingsSimulated) and explains how simulated ring models share functionality with real ring models through the base class architecture.
277-359: LGTM!The example JSON file is complete and demonstrates valid structure for all feature types (stars, bodies, rings). This provides a useful reference for users creating simulated image configurations.
docs/user_guide_navigation.rst (3)
99-102: LGTM!Good refactoring to use cross-references to the new
introduction_configurationdocument rather than duplicating configuration details here.
140-147: LGTM!The updated navigation options properly document
ringsas a valid model name and clarify that typically only one technique should be used at a time. This aligns with the PR's ring model implementation.
280-286: LGTM!The new "Simulated Images" section provides a clean integration point with the detailed simulated images documentation, following the pattern of using cross-references to avoid content duplication.
src/nav/nav_master/nav_master.py (1)
281-329: LGTM! Well-structured ring model computation with clear separation of simulated vs configured paths.The implementation correctly:
- Uses early returns for guard conditions (no closest planet, no ring definitions)
- Distinguishes between simulated rings (per-ring models with
ring:<name>naming) and configured rings (single model withringsnaming)- Properly updates metadata for both paths
One minor observation: the model naming convention differs between paths (
ring:<name>for simulated,ringsfor configured), which is intentional for filtering purposes but worth documenting if not already.src/nav/nav_model/nav_model_rings_base.py (3)
32-71: Anti-aliasing implementation looks correct with documented compatibility note.The shade computation logic is sound:
- Correctly handles
shade_abovedirection with sign flipping- The comment on lines 65-66 about the old code behavior (
shade[shade > 1.] = 0.) being potentially buggy but kept for compatibility is helpful documentation.
73-189: Edge fade computation with thorough pixel-coverage case analysis.The implementation correctly handles all four cases for both
shade_abovedirections:
- Edge and fade end both within pixel
- Edge within pixel, fade end extends beyond
- Edge before pixel, fade end within pixel
- Full pixel coverage
The nested integral functions (
int_func/int_func2) appropriately captureedge_radiusandfade_widthfrom the enclosing scope.
244-311: Well-designed edge tangent detection using variance-based heuristic.The approach of using weighted variance (lines 270-271) to determine edge direction is robust and avoids the cancellation issues that would occur with simple averaging when edge points are on both sides of the sample point.
The label placement logic (lines 290-311) correctly:
- Places labels perpendicular to edge direction
- Uses image center to determine which side (left/right or top/bottom) to place labels
src/nav/sim/sim_ring.py (2)
16-59: Good refactoring:compute_edge_radius_mode1now delegates to shared ellipse formula.This eliminates the code duplication that was previously flagged, with
compute_edge_radius_mode1computing the angle and delegating tocompute_edge_radius_at_anglefor the actual ellipse calculation.
384-441: Ring rendering logic correctly handles all edge configurations.The implementation properly handles:
- RINGLET with both edges: Either fills solid region (line 406) or applies shading on both sides when
shade_solid=True(lines 386-398)- RINGLET with single edge: Applies fade in appropriate direction (lines 407-418)
- GAP: Correctly subtracts brightness using fade factors (lines 421-441)
The in-place modification of
imgviaimg[:] = np.clip(...)is efficient and correctly documented.src/nav/nav_model/nav_model_rings_simulated.py (1)
142-229: Well-structured edge annotation creation for simulated rings.The implementation correctly:
- Extracts mode 1 parameters for inner/outer edges (lines 175-176)
- Computes edge masks in data coordinates using
compute_border_atop_simulated- Embeds masks into extended FOV coordinates (lines 196-199, 221-224)
- Delegates to the unified base class method for annotation creation
The label generation (
IER/OERfor ringlets,IEG/OEGfor gaps) matches the convention used inNavModelRings.src/nav/nav_model/nav_model_rings.py (1)
649-659: Intentional override with extended signature for conflict-aware fading.The
# type: ignore[override]comment correctly documents that this method intentionally differs from the base class signature. The extended parameters (radius_width_km,min_radius_width_km,feature_list_by_a) enable the conflict-checking logic (lines 685-699) that dynamically adjusts fade width when features are too close together.src/nav/sim/render.py (4)
12-13: LGTM!New imports for ring rendering and type annotations are correctly added and used throughout the file.
314-400: LGTM!Well-structured helper function that properly encapsulates single-body rendering logic. The parameter handling, shape caching, and translation logic are consistent with the existing
_render_bodies_positioned_cachedimplementation. Good extraction for reuse in the combined rendering pipeline.
613-620: LGTM!The default range assignment for rings (starting at 1000.0) provides reasonable separation from body defaults, ensuring rings render behind bodies by default. The in-place modification is safe since the cached function returns deepcopied results to callers.
735-745: LGTM!Metadata structure is properly extended with
ringsandring_masksfields, enabling downstream consumers to access ring information and perform hit-testing on rings similar to body handling.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/nav_create_simulated_image.py (1)
2143-2171: Range validation/warnings don’t match behavior: rings participate in range-based ordering but duplicates aren’t flagged.Selection/rendering sorts bodies+rings by
range, but_validate_ranges()only checks bodies. Either validate both (and cross-type collisions), or rename the warning/UI copy to “duplicate body ranges”.Proposed change (validate bodies + rings)
def _validate_ranges(self) -> None: - """Check for duplicate body ranges and display a warning if found.""" - ranges = [] - for i in range(len(self.sim_params['bodies'])): - range_val = self.sim_params['bodies'][i].get('range') - if range_val is not None: - ranges.append(float(range_val)) - duplicates = len(ranges) != len(set(ranges)) - self._warning_label.setText( - 'Warning: duplicate body ranges' if duplicates else '' - ) + """Check for duplicate ranges across bodies and rings.""" + ranges: list[float] = [] + for b in self.sim_params.get('bodies', []): + rv = b.get('range') + if rv is not None: + ranges.append(float(rv)) + for r in self.sim_params.get('rings', []): + rv = r.get('range') + if rv is not None: + ranges.append(float(rv)) + duplicates = len(ranges) != len(set(ranges)) + self._warning_label.setText('Warning: duplicate ranges' if duplicates else '')Also applies to: 2248-2296
🤖 Fix all issues with AI agents
In `@docs/introduction_overview.rst`:
- Around line 63-100: The docs incorrectly state that the standard programs can
generate cloud tasks JSON using --output-cloud-tasks-file; update the text to
accurately reflect that only nav_offset supports the --output-cloud-tasks-file
option (not nav_backplanes or nav_create_bundle) by changing the sentence to
reference nav_offset explicitly, or alternatively implement the option in
nav_backplanes and nav_create_bundle if intended; refer to the commands
nav_offset, nav_backplanes, nav_create_bundle and the flag
--output-cloud-tasks-file when making the correction.
In `@src/main/nav_create_simulated_image.py`:
- Around line 1357-1362: The lambda connected to delete_btn.clicked uses an
unused parameter named checked which triggers lint noise; update the lambda
parameter to a throwaway name (e.g., _checked or _) where the signal handler is
defined (the lambda that calls self._delete_tab_by_index('body', i)) and do the
same for the other occurrence that calls self._delete_tab_by_index (the block
around lines 1584-1588) so the unused clicked-signal arg is clearly ignored.
- Around line 1760-1807: The handlers (_on_body_crater_fill_slider,
_on_body_crater_fill_spin, _on_body_anti_aliasing_slider,
_on_body_anti_aliasing_spin) assume _find_tab_by_properties returns a valid tab
index and call self._tabs.widget(tab_idx) unguarded; change each handler to
check that tab_idx is not None (and >= 0) before accessing tab widget or its
controls, and early-return if the tab isn't found so you don't call widget(...)
on an invalid index; keep the existing sim_params index guard and
updater.request_update() only when both the tab exists and the body index is in
range.
In `@src/nav/nav_model/nav_model_rings_base.py`:
- Around line 32-71: The clamp logic in _compute_antialiasing currently sets
values above 1.0 to 0.0 which is incorrect; change the upper clamp so
shade[shade > 1.0] = 1.0 (so values >1 become full coverage) and ensure the
result is consistent with the implementation in sim_ring.py (update sim_ring.py
if it still differs), then run/adjust the antialiasing tests (e.g., tests
referencing lines ~127/135) to expect the corrected clamp semantics.
- Around line 325-328: The call to ndimage.maximum_filter when creating
text_avoid_mask uses label_mask_enlarge as the size argument which will raise a
TypeError if it is a float; update the code around maximum_filter (affecting
text_avoid_mask, model_mask, label_mask_enlarge) to ensure label_mask_enlarge is
converted to an integer (e.g., using int() or validation) and optionally
validate/configure earlier that label_mask_enlarge is a positive integer before
passing it into maximum_filter so SciPy receives an int or tuple of ints.
In `@src/nav/nav_model/nav_model_rings.py`:
- Around line 607-617: The override of _compute_edge_fade in NavModelRings has a
different signature than NavModelRingsBase._compute_edge_fade, which can break
polymorphism; either make NavModelRings._compute_edge_fade match the base
signature by moving the extra parameters (feature_list_by_a, resolutions,
radius_width_km, min_radius_width_km, shade_above, edge_radius) into optional
parameters with sensible defaults, or rename the variant in NavModelRings to a
distinct name (e.g., _compute_edge_fade_conflict_aware) and update any internal
callers to use the new name; ensure NavModelRings still implements a compatible
_compute_edge_fade(method) so base-class callers continue to work and keep
references to NavModelRingsBase._compute_edge_fade and
NavModelRings._compute_edge_fade consistent.
In `@src/nav/sim/render.py`:
- Around line 669-696: render_ring mutates its input in-place (adding for
RINGLET, subtracting for GAP), but the current loop assigns the temporary buffer
into img (img[ring_mask] = ...), which discards the original background/stars;
instead compute the per-pixel delta by comparing the rendered buffer to the
original background (e.g., delta = ring_img - background_for_ring for RINGLET,
or delta = background_for_gap - temp_bg for GAP) and apply that delta to img
(img[mask] += delta[mask] or img[mask] -= delta[mask]) so composition preserves
underlying pixels; update the RINGLET branch (using ring_img and the original
background) and the GAP branch (using temp_bg and background=1.0) to apply
deltas and still populate ring_mask_map[orig_idx] with the mask.
In `@src/nav/sim/sim_ring.py`:
- Around line 93-99: The comment containing the elliptical orbit equation uses
the Unicode character 'ν' which ruff flags as ambiguous; update that comment to
use ASCII 'nu' instead (e.g., change "ν" to "nu" in the formula comment near the
computation of r in sim_ring.py where variables a, ae, e, true_anomaly are
used), and make the same replacement in the other comment block around the code
at the section that references true_anomaly (the block you noted at the second
occurrence). Ensure only the comment text is changed—do not modify variable
names or logic.
- Around line 191-200: The edge-radius computation is duplicated in
compute_border_atop_simulated; replace the block that computes e,
days_since_epoch, long_peri_rad, true_anomaly and edge_radii with a call to the
shared helper _compute_edge_radii_array, passing the same inputs (angles, a, ae,
epoch, time, long_peri, rate_peri) so all eccentricity/precession logic lives in
one place; ensure the call returns the edge_radii used later and preserves the
existing clipping of e (<1.0) and behavior for a == 0.0.
♻️ Duplicate comments (3)
src/nav/sim/sim_ring.py (1)
95-98: Eccentricity clamping is silent; consider warning/logging.This was previously raised: clamping
e >= 1.0to0.99can mask bad config/inputs. If you still want to keep the clamp, at least emit a warning includinga/ae/e/time/epoch.Also applies to: 136-139
src/nav/nav_model/nav_model_rings.py (1)
312-376: Don’t catch broadExceptionfor date parsing.
utc_to_et()failures should be narrowly handled (e.g.,ValueError,TypeError) so unexpected errors aren’t silently treated as “bad config”.src/main/nav_create_simulated_image.py (1)
261-275: Crash on load:closest_planetcan be None but is passed tofindText().
_on_closest_planet()storesNonefor empty;_load_parameters()assigns'closest_planet': params.get('closest_planet')which can beNone; thenfindText(closest_planet)will raise.Proposed change
self._closest_planet_combo = QComboBox() self._closest_planet_combo.setEditable(True) - self._closest_planet_combo.addItems(['MERCURY', 'VENUS', 'EARTH', 'MARS', 'JUPITER', + # Index 0 represents "no selection" + self._closest_planet_combo.addItems(['', 'MERCURY', 'VENUS', 'EARTH', 'MARS', 'JUPITER', 'SATURN', 'URANUS', 'NEPTUNE', 'PLUTO']) @@ - closest_planet = self.sim_params.get('closest_planet', 'SATURN') - index = self._closest_planet_combo.findText(closest_planet) + closest_planet = str(self.sim_params.get('closest_planet') or '') + index = self._closest_planet_combo.findText(closest_planet) if index >= 0: self._closest_planet_combo.setCurrentIndex(index) else: self._closest_planet_combo.setCurrentText(closest_planet)Also applies to: 607-613, 2442-2464
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
docs/introduction.rstdocs/introduction_configuration.rstdocs/introduction_overview.rstdocs/introduction_simulated_images.rstdocs/user_guide.rstsrc/main/nav_create_simulated_image.pysrc/nav/nav_model/nav_model_body_simulated.pysrc/nav/nav_model/nav_model_combined.pysrc/nav/nav_model/nav_model_rings.pysrc/nav/nav_model/nav_model_rings_base.pysrc/nav/obs/obs_inst_sim.pysrc/nav/sim/render.pysrc/nav/sim/sim_ring.pysrc/nav/ui/manual_nav_dialog.py
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-24T19:48:51.676Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 48
File: main/nav_main_offset.py:167-171
Timestamp: 2025-11-24T19:48:51.676Z
Learning: In the nav_main_offset.py file, it is normal and expected for the nav_default_config.yaml file to be missing. The FileNotFoundError suppression when attempting to load this default config is intentional behavior, not an error condition that needs logging.
Applied to files:
docs/introduction_configuration.rst
📚 Learning: 2025-11-14T03:01:15.014Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 37
File: nav/nav_model/nav_model_body.py:251-271
Timestamp: 2025-11-14T03:01:15.014Z
Learning: In the simulated body navigation model (`NavModelBody._create_simulated_model` in nav/nav_model/nav_model_body.py), the model intentionally does NOT include `sim_offset_v/u` from the observation. This deliberate misalignment between the simulated observation image (which includes offsets) and the navigation model (which uses base centers) is designed to test the navigation correlation process's ability to recover the offset, mimicking real-world positional uncertainty.
Applied to files:
src/nav/nav_model/nav_model_body_simulated.pydocs/introduction_simulated_images.rst
📚 Learning: 2026-01-14T03:10:15.891Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 80
File: src/nav/nav_model/nav_model_body_simulated.py:36-37
Timestamp: 2026-01-14T03:10:15.891Z
Learning: In `nav.sim.sim_body.create_simulated_body` (used in `nav_model_body_simulated.py`), the `anti_aliasing` parameter accepts values in the range 0 to 1, where 1 represents maximum anti-aliasing, not a supersampling factor like 1×, 2×, 4×, etc.
Applied to files:
src/nav/nav_model/nav_model_body_simulated.py
📚 Learning: 2025-11-15T01:39:53.839Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 37
File: nav/support/sim.py:138-139
Timestamp: 2025-11-15T01:39:53.839Z
Learning: In nav/support/sim.py, the crater area calculation intentionally uses `crater_max_radius` (not the arithmetic average of min and max) because the power law distribution controlled by `crater_power_law_exponent` favors smaller crater sizes, making the max radius a better proxy for effective area coverage.
Applied to files:
src/nav/nav_model/nav_model_body_simulated.py
📚 Learning: 2025-12-23T19:46:29.815Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 68
File: src/nav/dataset/dataset_pds3_cassini_iss.py:241-241
Timestamp: 2025-12-23T19:46:29.815Z
Learning: In Python, for staticmethod methods, super() cannot be used because there is no class or instance context. If a subclass static method needs to call a parent class's static method, invoke it directly as ParentClass.method_name() rather than super().method_name(). This applies broadly to Python files in the repository; ensure the parent method is defined as a static method or accessible in the class hierarchy and that imports are correct.
Applied to files:
src/nav/nav_model/nav_model_body_simulated.pysrc/nav/nav_model/nav_model_rings_base.pysrc/nav/nav_model/nav_model_combined.pysrc/nav/sim/sim_ring.pysrc/nav/ui/manual_nav_dialog.pysrc/nav/nav_model/nav_model_rings.pysrc/nav/obs/obs_inst_sim.pysrc/nav/sim/render.pysrc/main/nav_create_simulated_image.py
🧬 Code graph analysis (4)
src/nav/nav_model/nav_model_rings.py (3)
src/nav/config/config.py (1)
rings(192-196)src/nav/support/time.py (2)
now_dt(17-24)utc_to_et(56-68)src/nav/nav_model/nav_model_rings_base.py (5)
_create_edge_annotations(191-341)_compute_edge_fade(73-189)int_func(105-110)int_func2(147-152)_compute_antialiasing(32-71)
src/nav/obs/obs_inst_sim.py (2)
src/nav/support/nav_base.py (1)
logger(38-40)src/nav/sim/render.py (1)
render_combined_model(748-776)
src/nav/sim/render.py (2)
src/nav/sim/sim_ring.py (1)
render_ring(266-437)src/nav/support/types.py (1)
MutableStar(21-55)
src/main/nav_create_simulated_image.py (2)
src/nav/obs/obs_snapshot.py (1)
closest_planet(302-303)src/nav/config/config.py (2)
bodies(185-189)rings(192-196)
🪛 Ruff (0.14.11)
src/nav/sim/sim_ring.py
93-93: Comment contains ambiguous ν (GREEK SMALL LETTER NU). Did you mean v (LATIN SMALL LETTER V)?
(RUF003)
134-134: Comment contains ambiguous ν (GREEK SMALL LETTER NU). Did you mean v (LATIN SMALL LETTER V)?
(RUF003)
309-309: Avoid specifying long messages outside the exception class
(TRY003)
src/nav/nav_model/nav_model_rings.py
87-87: Boolean-typed positional argument in function definition
(FBT001)
88-88: Boolean-typed positional argument in function definition
(FBT001)
89-89: Boolean-typed positional argument in function definition
(FBT001)
112-112: Logging statement uses f-string
(G004)
120-120: Avoid specifying long messages outside the exception class
(TRY003)
125-125: Avoid specifying long messages outside the exception class
(TRY003)
129-129: Avoid specifying long messages outside the exception class
(TRY003)
131-132: Logging statement uses f-string
(G004)
151-151: Logging statement uses f-string
(G004)
166-166: Logging statement uses f-string
(G004)
281-281: Boolean positional value in function call
(FBT003)
293-293: Boolean positional value in function call
(FBT003)
345-345: Do not catch blind exception: Exception
(BLE001)
347-348: Logging statement uses f-string
(G004)
356-356: Do not catch blind exception: Exception
(BLE001)
358-359: Logging statement uses f-string
(G004)
374-374: Logging statement uses f-string
(G004)
382-382: Logging statement uses f-string
(G004)
422-422: Logging statement uses f-string
(G004)
428-428: Logging statement uses f-string
(G004)
434-434: Logging statement uses f-string
(G004)
454-455: Logging statement uses f-string
(G004)
598-598: Boolean positional value in function call
(FBT003)
650-651: Logging statement uses f-string
(G004)
655-656: Logging statement uses f-string
(G004)
783-783: Logging statement uses f-string
(G004)
794-794: Logging statement uses f-string
(G004)
798-799: Logging statement uses f-string
(G004)
804-804: Boolean positional value in function call
(FBT003)
869-870: Logging statement uses f-string
(G004)
878-879: Logging statement uses f-string
(G004)
896-896: Logging statement uses f-string
(G004)
904-905: Logging statement uses f-string
(G004)
src/main/nav_create_simulated_image.py
1009-1009: Avoid specifying long messages outside the exception class
(TRY003)
1060-1060: Avoid specifying long messages outside the exception class
(TRY003)
1360-1360: Unused lambda argument: checked
(ARG005)
1587-1587: Unused lambda argument: checked
(ARG005)
1765-1765: Boolean positional value in function call
(FBT003)
1767-1767: Boolean positional value in function call
(FBT003)
1777-1777: Boolean positional value in function call
(FBT003)
1779-1779: Boolean positional value in function call
(FBT003)
1789-1789: Boolean positional value in function call
(FBT003)
1791-1791: Boolean positional value in function call
(FBT003)
1801-1801: Boolean positional value in function call
(FBT003)
1803-1803: Boolean positional value in function call
(FBT003)
1808-1808: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
1866-1866: Boolean-typed positional argument in function definition
(FBT001)
1898-1898: Boolean positional value in function call
(FBT003)
1900-1900: Boolean positional value in function call
(FBT003)
1933-1933: Boolean-typed positional argument in function definition
(FBT001)
1965-1965: Boolean positional value in function call
(FBT003)
1967-1967: Boolean positional value in function call
(FBT003)
2021-2021: Boolean positional value in function call
(FBT003)
2023-2023: Boolean positional value in function call
(FBT003)
2027-2027: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
2255-2255: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
2256-2256: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
2379-2379: Avoid specifying long messages outside the exception class
(TRY003)
2523-2523: Dynamically typed expressions (typing.Any) are disallowed in state
(ANN401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test nav (ubuntu-latest, 3.10)
- GitHub Check: Test nav (ubuntu-latest, 3.13)
- GitHub Check: Test nav (ubuntu-latest, 3.11)
- GitHub Check: Test nav (ubuntu-latest, 3.12)
🔇 Additional comments (18)
src/nav/ui/manual_nav_dialog.py (3)
30-30: LGTM!The import of custom NumPy type aliases is appropriate and follows the existing import organization.
33-44: LGTM!The updated type hints accurately document the function contract (float input → uint8 output), and the explicit cast on line 44 is necessary to satisfy the custom type alias since
astype()returns a generic ndarray type.
47-47: LGTM!The
NDArrayFloatTypeannotation correctly documents that the function expects a float array input, which aligns with its usage for sampling the correlation surface.src/nav/nav_model/nav_model_combined.py (2)
86-110: LGTM on the model combination logic.The per-pixel model selection via
np.argmin(ranges_arr, axis=0)correctly picks the closest model for each pixel, and the index lookup properly maps back to original model indices. The infinity initialization for non-masked pixels in the new scalar-range handling ensures these pixels don't interfere with the minimum calculation.
74-83: This review comment identifies a concern that does not exist. All models in the codebase (rings, body, stars, etc.) returnmodel_imgandmodel_maskas extended FOV sized arrays created viaobs.make_extfov_zeros(). Sincemodel.model_mask.shapeis validated to matchmodel.model_img.shape(lines 58-60), and both are guaranteed to be extended FOV sized, the boolean indexing on line 78 (rng_arr[model.model_mask] = rng) operates on arrays of the same shape. No additional shape validation is required.src/nav/nav_model/nav_model_body_simulated.py (2)
27-42: Docstring improvement looks good.The restructured docstring now clearly separates expected keys from ignored keys, making the API contract clearer. The note about
anti_aliasingalways being set to max (1) is consistent with the implementation. Based on learnings,anti_aliasing=1correctly represents maximum anti-aliasing on the 0-1 scale.
144-145: Range simplification from array to scalar is correct and intentional.The base class (
nav_model.py) explicitly defines_rangewith the type annotationNDArrayFloatType | float | Noneand its docstring states: "Returns the range to the target body, either as a single value or per-pixel array." This polymorphic design is intentional and already supported by consumers of the property.Using a scalar value (
np.inf) for simulated bodies is consistent with other simulated models in the codebase (e.g.,nav_model_stars.pyandnav_model_rings_simulated.py), while realistic models use per-pixel arrays. No compatibility issues exist.src/nav/obs/obs_inst_sim.py (2)
86-87: New time/epoch fields correctly support ring calculations.The
sim_timeandsim_epochfields are properly extracted with safe defaults. These are needed for ring edge calculations involving precession rates.
89-96: Ring metadata integration follows established patterns.The addition of
sim_ringsfollows the same pattern as existingsim_star_listandsim_body_models, maintaining consistency. The comment update to "Render combined model" accurately reflects that rendering now includes stars, bodies, and rings.docs/introduction_configuration.rst (1)
1-152: Comprehensive configuration documentation.This documentation clearly explains the three-layer configuration system and provides practical examples. The inclusion of
config_20_saturn_rings.yamlandringsas a valid--nav-modelsentry aligns well with the ring model implementation in this PR.docs/introduction_simulated_images.rst (4)
1-109: Well-documented simulated image JSON schema.The top-level fields documentation is comprehensive and clearly explains each parameter's purpose, type, and default value. The inclusion of
time,epoch, andclosest_planetfields properly documents the ring simulation capabilities.
182-228: Clear ring parameter documentation with edge mode details.The ring parameters section thoroughly documents the required fields and the edge mode data structure. Noting that
rmsis "not currently used in rendering" (line 218-219) is helpful for users understanding the current implementation scope.
248-275: Accurate class hierarchy documentation.The ring navigation model architecture explanation correctly describes the base class (
NavModelRingsBase) and its two subclasses (NavModelRingsfor real rings,NavModelRingsSimulatedfor simulated rings), aligning with the implementation introduced in this PR.
277-359: Example JSON structure is valid and accurately demonstrates the complete ring, body, and star configuration.The example properly demonstrates all required and optional parameters for stars, bodies, and rings. The ring parameters correctly include
feature_type,center_v,center_u,shading_distance,inner_data, andouter_datawith mode 1 parameters (a,ae,long_peri,rate_peri). The optionalrmsparameter is correctly documented in the guide as "not currently used in rendering" and its inclusion here is appropriate. All parameter values are properly typed and the nested structure matches the rendering pipeline's expectations.docs/user_guide.rst (1)
1-19: All referenced documentation files exist and are properly configured.The user guide structure is well-organized with appropriate sections covering navigation workflows, backplanes, PDS4 bundle creation, and instrument-specific appendices. All seven toctree references point to existing .rst files in the docs/ directory.
docs/introduction.rst (1)
1-16: LGTM! Well-structured introduction documentation.The reStructuredText syntax is correct, and the toctree properly organizes the introduction sub-sections. The content provides a clear entry point for users to understand the system.
docs/introduction_overview.rst (2)
1-26: LGTM! Clear and accurate overview.The overview section and navigation pipeline description are well-written and properly align with the PR's implementation of ring models. The mention of "rings" on Line 16 correctly reflects the new functionality being added.
28-52: LGTM! Standard installation instructions.The installation section provides clear instructions for both
pipandpipx, with appropriate explanations of when to use each method.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/nav_create_simulated_image.py`:
- Around line 1774-1794: The method _on_body_crater_fill_spin contains a
duplicated block that re-uses tab_w and slider outside the tab_w None check,
risking UnboundLocalError; remove the second duplicate block (the lines that
reassign slider = tab_w.crater_fill_slider, blockSignals/setValue/blockSignals
and the repeated sim_params/_updater updates) so that slider UI updates occur
only inside the if tab_w is not None branch and sim_params/_updater updates
remain a single, guarded update using
self.sim_params['bodies'][idx]['crater_fill'] and
self._updater.request_update().
♻️ Duplicate comments (2)
src/nav/sim/sim_ring.py (2)
102-141: Replace ambiguous Unicode character ν with ASCII 'nu'.Line 133 uses the Greek letter ν (nu) which is visually ambiguous with the Latin letter v. This is flagged by static analysis (RUF003). Use ASCII 'nu' in comments for clarity.
🔧 Suggested fix
- # Compute radius using elliptical orbit equation: r = a(1 - e^2) / (1 + e*cos(ν)) + # Compute radius using elliptical orbit equation: r = a(1 - e^2) / (1 + e*cos(nu))
62-100: Silent eccentricity clamping may mask configuration errors.When
e >= 1.0(parabolic/hyperbolic orbit), the code silently clamps to 0.99. Consider logging a warning so users are aware their configuration resulted in an invalid eccentricity value.🔧 Suggested enhancement
e = ae / a if a > 0 else 0.0 if e >= 1.0: + import logging + logging.warning(f'Eccentricity {e:.4f} >= 1.0 (ae={ae}, a={a}), clamping to 0.99') e = 0.99 # Clamp eccentricity to valid range
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
docs/introduction_overview.rstdocs/user_guide_configuration.rstdocs/user_guide_simulated_images.rstsrc/main/nav_create_simulated_image.pysrc/nav/nav_model/nav_model_rings_base.pysrc/nav/sim/sim_ring.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-14T03:01:15.014Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 37
File: nav/nav_model/nav_model_body.py:251-271
Timestamp: 2025-11-14T03:01:15.014Z
Learning: In the simulated body navigation model (`NavModelBody._create_simulated_model` in nav/nav_model/nav_model_body.py), the model intentionally does NOT include `sim_offset_v/u` from the observation. This deliberate misalignment between the simulated observation image (which includes offsets) and the navigation model (which uses base centers) is designed to test the navigation correlation process's ability to recover the offset, mimicking real-world positional uncertainty.
Applied to files:
docs/user_guide_simulated_images.rst
📚 Learning: 2026-01-14T04:39:08.699Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 80
File: src/nav/nav_model/nav_model_rings.py:873-922
Timestamp: 2026-01-14T04:39:08.699Z
Learning: In nav_model_rings.py, the fade rendering in _render_single_edge intentionally uses edge_radii.filled(0.0) without masking invalid backplane pixels because the fade calculation handles partial pixel coverage and can legitimately extend into masked areas. Reviewers should preserve this behavior where the fade math can spill over and ensure comments/docs justify it; verify tests cover this behavior and avoid masking in similar edge cases unless the fade logic is changed.
Applied to files:
src/nav/nav_model/nav_model_rings_base.pysrc/nav/sim/sim_ring.py
📚 Learning: 2025-12-23T19:46:29.815Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 68
File: src/nav/dataset/dataset_pds3_cassini_iss.py:241-241
Timestamp: 2025-12-23T19:46:29.815Z
Learning: In Python, for staticmethod methods, super() cannot be used because there is no class or instance context. If a subclass static method needs to call a parent class's static method, invoke it directly as ParentClass.method_name() rather than super().method_name(). This applies broadly to Python files in the repository; ensure the parent method is defined as a static method or accessible in the class hierarchy and that imports are correct.
Applied to files:
src/nav/nav_model/nav_model_rings_base.pysrc/nav/sim/sim_ring.pysrc/main/nav_create_simulated_image.py
🧬 Code graph analysis (2)
src/nav/nav_model/nav_model_rings_base.py (3)
src/nav/annotation/annotation.py (2)
Annotation(11-106)avoid_mask(85-87)src/nav/annotation/annotations.py (2)
Annotations(13-183)add_annotations(26-54)src/nav/annotation/annotation_text_info.py (6)
AnnotationTextInfo(48-334)text_loc(82-84)ref_vu(87-89)font(97-99)font_size(102-104)color(92-94)
src/main/nav_create_simulated_image.py (3)
src/nav/obs/obs_snapshot.py (1)
closest_planet(302-303)src/nav/annotation/annotation_text_info.py (1)
text(77-79)src/nav/config/config.py (2)
bodies(185-189)rings(192-196)
🪛 Ruff (0.14.11)
src/nav/sim/sim_ring.py
133-133: Comment contains ambiguous ν (GREEK SMALL LETTER NU). Did you mean v (LATIN SMALL LETTER V)?
(RUF003)
312-312: Avoid specifying long messages outside the exception class
(TRY003)
src/main/nav_create_simulated_image.py
1009-1009: Avoid specifying long messages outside the exception class
(TRY003)
1060-1060: Avoid specifying long messages outside the exception class
(TRY003)
1767-1767: Boolean positional value in function call
(FBT003)
1769-1769: Boolean positional value in function call
(FBT003)
1781-1781: Boolean positional value in function call
(FBT003)
1783-1783: Boolean positional value in function call
(FBT003)
1788-1788: Boolean positional value in function call
(FBT003)
1790-1790: Boolean positional value in function call
(FBT003)
1802-1802: Boolean positional value in function call
(FBT003)
1804-1804: Boolean positional value in function call
(FBT003)
1816-1816: Boolean positional value in function call
(FBT003)
1818-1818: Boolean positional value in function call
(FBT003)
1823-1823: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
1881-1881: Boolean-typed positional argument in function definition
(FBT001)
1913-1913: Boolean positional value in function call
(FBT003)
1915-1915: Boolean positional value in function call
(FBT003)
1948-1948: Boolean-typed positional argument in function definition
(FBT001)
1980-1980: Boolean positional value in function call
(FBT003)
1982-1982: Boolean positional value in function call
(FBT003)
2036-2036: Boolean positional value in function call
(FBT003)
2038-2038: Boolean positional value in function call
(FBT003)
2042-2042: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
2270-2270: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
2271-2271: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
2394-2394: Avoid specifying long messages outside the exception class
(TRY003)
2538-2538: Dynamically typed expressions (typing.Any) are disallowed in state
(ANN401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test nav (ubuntu-latest, 3.13)
- GitHub Check: Test nav (ubuntu-latest, 3.10)
- GitHub Check: Test nav (ubuntu-latest, 3.11)
- GitHub Check: Test nav (ubuntu-latest, 3.12)
🔇 Additional comments (31)
docs/introduction_overview.rst (1)
1-100: Excellent documentation - previous concern has been addressed.This is a well-structured and clear overview document. The previous review flagged that earlier text incorrectly implied multiple programs support
--output-cloud-tasks-file, but the current version (lines 99-100) correctly specifies that onlynav_offsetsupports this option. The document accurately describes the navigation pipeline, installation methods, and command-line tools, with terminology that aligns well with the PR's ring model implementation.docs/user_guide_configuration.rst (6)
1-14: LGTM! Clear introduction to the configuration system.The introduction effectively establishes the hierarchical YAML-based configuration system and clearly sets up the loading order explanation that follows.
30-32: LGTM! Past review comment successfully addressed.The documentation now includes
config_95_pds4.yamlwith a clear description of its purpose, resolving the issue raised in the previous review.
34-65: LGTM! Well-structured configuration file documentation.The user configuration section clearly explains the
nav_default_config.yamllocation and usage. The YAML examples are properly formatted and effectively demonstrate the configuration file structure and override behavior.
67-134: LGTM! Comprehensive coverage of configuration override mechanisms.The documentation provides clear, actionable guidance for:
- Creating custom user configuration files with minimal overrides
- Using command-line configuration files for per-run customization
- Leveraging command-line options for highest-priority overrides
The explicit precedence hierarchy (lines 132-134) and the inclusion of the
ringsmodel option (line 126) align well with the ring model implementation objectives of this PR.
136-152: LGTM! Effective example demonstrating configuration precedence.The example clearly illustrates how the configuration hierarchy works in practice, using concrete values (128 → 256 → 512) that make the override sequence easy to understand. The additional
--nav-modelsexample effectively demonstrates how command-line options take ultimate precedence.
23-23: No action needed. The description "Saturn ring system parameters" accurately reflects the file's content. The config_20_saturn_rings.yaml file contains only Saturn ring configuration (no Uranus ring data present). If Uranus rings are supported elsewhere in the PR, they would be configured in a separate file.Likely an incorrect or invalid review comment.
docs/user_guide_simulated_images.rst (5)
1-109: Documentation structure and top-level fields look good.The documentation provides a comprehensive overview of the simulated images workflow, including GUI usage, JSON format, and field descriptions. The structure is well-organized and the field descriptions are clear.
110-181: Star and body parameter documentation is accurate.The parameter descriptions for stars and bodies are complete and match the implementation in the GUI code.
182-228: Ring parameter documentation is comprehensive.The ring parameters section accurately documents the inner/outer edge mode data structure with mode 1 requirements. The field descriptions align with the implementation in
sim_ring.pyand the GUI.
248-276: Ring navigation model class hierarchy is accurately documented.The documentation correctly describes the inheritance relationship between
NavModelRingsBase,NavModelRings, andNavModelRingsSimulated, and explains how simulated ring models read parameters from the JSON file.
277-359: Example JSON is comprehensive and correct.The example JSON file demonstrates all sections including stars, bodies, and rings with proper mode 1 data structures. This serves as a good reference for users.
src/nav/nav_model/nav_model_rings_base.py (5)
1-31: Module structure and imports are clean.The base class is well-documented with clear docstrings explaining its purpose as a shared base for both real and simulated ring models.
32-71: Anti-aliasing computation correctly clamps to [0, 1].The implementation now correctly clamps shade values:
shade[shade < 0.0] = 0.0andshade[shade > 1.0] = 1.0. This is the correct behavior for anti-aliasing coverage, unlike the previously flagged bug where values > 1.0 were set to 0.0.
72-106: Edge annotation method signature and early setup are well-structured.The method properly handles empty edge lists and retrieves configuration from
self._config.rings. The edge_info_list tuple now correctly uses_edge_typeto indicate intentional non-use.
107-169: Edge point sampling and tangent direction calculation are reasonable.The algorithm samples up to 50 edge points, computes tangent direction using weighted variance of nearby points, and determines label placement based on whether the edge runs more horizontally or vertically. The weighted variance approach is a sensible heuristic for determining edge direction.
170-222: Label placement logic and annotation creation are correct.The code properly determines label placement (left/right for vertical edges, top/bottom for horizontal edges) based on position relative to image center. The
int(label_mask_enlarge)cast at line 208 addresses the previously flagged scipymaximum_filterfloat issue.src/nav/sim/sim_ring.py (5)
1-60: compute_edge_radius_mode1 correctly delegates to shared ellipse formula.The refactoring to delegate to
compute_edge_radius_at_anglereduces code duplication and keeps the ellipse formula in one place.
143-233: Border computation logic is correct.The
compute_border_atop_simulatedfunction correctly computes pixel-center coordinates, edge radii using the shared helper, and detects border pixels via sign-change detection between neighbors. The refactoring to use_compute_edge_radii_arrayreduces duplication.
235-267: Anti-aliasing and fade factor implementations are correct.The
_compute_antialiasing_shadefunction properly computes shade values in [0, 1]. The_compute_fade_factorfunction correctly handles theshading_distance <= 0.0edge case with a step-function fallback, addressing the previously flagged divide-by-zero concern.
269-340: render_ring function setup and edge extraction are well-implemented.The function properly extracts mode 1 data using the
next()pattern for consistency, validates that at least one edge is specified, and sets up coordinate grids correctly.
341-440: Ring rendering logic handles all edge configurations correctly.The function properly handles:
- RINGLET with both edges (fill or shade_solid mode)
- RINGLET with single edge (inner-only or outer-only)
- GAP with both edges or single edge
The anti-aliasing and fading are applied correctly based on feature type and edge configuration. The in-place image modification with clipping to [0, 1] is appropriate.
src/main/nav_create_simulated_image.py (9)
104-128: Data model correctly includes ring support.The
sim_paramsdictionary now includesrings,time,ring_epoch,closest_planet, andshade_solid_ringsfields to support the ring feature integration.
261-293: New ring-related UI controls are well-implemented.The closest planet combo, time, and epoch spin boxes are properly configured with appropriate ranges and connected to their handlers.
884-923: Ring tab addition follows the same pattern as body and star tabs.The
_add_ring_tabmethod correctly creates default ring parameters with inner and outer edge mode 1 data, uses unique naming, and rebuilds tabs consistently.
1367-1592: Ring tab building is comprehensive with proper edge controls.The
_build_ring_tabmethod creates a complete UI for ring editing including:
- Name, feature type, center coordinates, range, shading distance
- Inner and outer edge checkboxes with enable/disable logic
- Mode 1 parameters (a, ae, long_peri, rate_peri, rms) for each edge
The control references are properly stored for later access.
1823-1880: Ring field handlers are correctly implemented.The
_on_ring_field,_on_ring_name,_get_or_create_mode1,_on_ring_inner_mode1, and_on_ring_outer_mode1handlers properly update the data model and trigger UI updates.
1881-2041: Edge enable/disable logic prevents invalid state.The
_on_ring_inner_enabledand_on_ring_outer_enabledhandlers correctly prevent both edges from being disabled simultaneously by re-checking the checkbox when the user attempts to disable the last enabled edge. The_validate_ring_edgesmethod provides an additional safeguard.
2262-2331: Model selection logic correctly handles rings.The
_select_model_atmethod now collects both bodies and rings with their ranges, sorts by range (near to far), and selects the topmost matching object. Stars are correctly treated as always behind bodies and rings.
2334-2396: Ring drag-move support is correctly implemented.The
_move_selected_bymethod now handles the 'ring' kind, updating center coordinates and syncing the tab spin boxes, following the same pattern as bodies and stars.
2432-2520: Load parameters correctly handles rings and new fields.The
_load_parametersmethod now loadstime,ring_epoch,closest_planet, andringsfrom JSON, and updates all corresponding UI controls.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/nav/nav_model/nav_model_rings.py`:
- Around line 437-457: Implement the missing mode-specific required-field
validation in the same function that currently checks mode['a'] (used by
_parse_mode_data): if mode_num == 1 require ['a', 'ae', 'long_peri',
'rate_peri'] else require ['amplitude', 'phase', 'pattern_speed']; for each
required field, if it is not in mode call self._logger.warning with a message
including feature_key, edge_type, i, mode_num and the missing field and return
False; keep the existing range checks for 'a' (min_radius/max_radius and
positive check) after verifying presence so KeyError cannot occur.
- Around line 474-478: The _get_base_radius function iterates mode_data and
assumes mode['a'] exists when mode['mode'] == 1, which can raise KeyError;
update _get_base_radius to check for the 'a' key (e.g., using 'a' in mode or
mode.get('a') is not None) before returning float(...) and otherwise skip that
entry or return None if missing, ensuring you reference mode_data and the
'mode'/'a' keys in the check and preserve the existing behavior of returning
None when no valid value is found.
In `@tests/nav/nav_model/test_nav_model_rings_antialiasing.py`:
- Around line 13-26: Add an explicit None return type annotation to the
constructor: update MockObservation.__init__ to declare "-> None" on the
signature so the method is defined as def __init__(self) -> None:, keeping the
same body and behavior; this addresses the static analysis requirement for the
MockObservation class.
♻️ Duplicate comments (6)
src/nav/nav_model/nav_model_rings.py (2)
343-369: Consider catching specific exceptions for date parsing.The bare
except Exceptionat lines 345 and 356 could hide unexpected errors. Sinceutc_to_etwraps Julian date parsing, consider catching more specific exceptions to avoid masking other issues.♻️ Proposed improvement
if start_date is not None: try: start_et = utc_to_et(start_date) - except Exception as e: + except (ValueError, TypeError) as e: self._logger.warning( f'Invalid start_date "{start_date}" for feature ' f'{feature_key}: {e}') continue ... if end_date is not None: try: end_et = utc_to_et(end_date) - except Exception as e: + except (ValueError, TypeError) as e: self._logger.warning( f'Invalid end_date "{end_date}" for feature ' f'{feature_key}: {e}') continue
494-527: Potential KeyError in _parse_mode_data for missing fields.The method directly accesses dictionary keys like
mode['a'],mode['ae'],mode['long_peri'],mode['rate_peri']for mode 1, andmode['amplitude'],mode['phase'],mode['pattern_speed']for other modes without checking existence. If the config is malformed, this will crash withKeyErrorrather than producing a clear validation error.Since
_validate_mode_datadoesn't currently enforce these fields (TODO comment at line 437), this creates a gap where invalid configs pass validation but crash during parsing.🐛 Proposed fix using .get() with logging
if mode_num == 1: # Mode 1: base radius with eccentricity - a = mode['a'] - ae = mode['ae'] - long_peri = mode['long_peri'] - rate_peri = mode['rate_peri'] + a = mode.get('a') + ae = mode.get('ae') + long_peri = mode.get('long_peri') + rate_peri = mode.get('rate_peri') + if any(v is None for v in (a, ae, long_peri, rate_peri)): + # Skip incomplete mode 1 entries + continue # ... rest of conversion else: - amplitude = mode['amplitude'] - phase = mode['phase'] - pattern_speed = mode['pattern_speed'] + amplitude = mode.get('amplitude') + phase = mode.get('phase') + pattern_speed = mode.get('pattern_speed') + if any(v is None for v in (amplitude, phase, pattern_speed)): + # Skip incomplete mode entries + continuesrc/main/nav_create_simulated_image.py (4)
1008-1010: Replaceassert Falsewithraise AssertionError().
assertstatements are stripped when Python runs with optimization flags (-O). This unreachable code path should raise an explicit exception to ensure it fails consistently in all execution modes.🔧 Suggested fix
- else: - raise AssertionError(f'Unknown kind: {kind}') + else: + raise AssertionError(f'Unknown kind: {kind}')Note: The code shows
raise AssertionErrorwhich is correct. If the actual code still hasassert False, please update it.
1774-1794: Critical bug: Duplicate code block accessestab_woutside the None check.Lines 1787-1793 duplicate the slider update logic from lines 1780-1783, but access
tab_wwithout theif tab_w is not Noneguard. Whentab_idx is None,tab_wwill be unbound, causing anUnboundLocalError. Even iftab_idxis valid buttab_wis somehowNone, this will crash withAttributeError.🐛 Proposed fix - remove duplicate code block
def _on_body_crater_fill_spin(self, idx: int, value: float) -> None: slider_val = int(value * 1000) tab_idx = self._find_tab_by_properties('body', idx) if tab_idx is not None: tab_w = self._tabs.widget(tab_idx) if tab_w is not None: slider = tab_w.crater_fill_slider # type: ignore slider.blockSignals(True) slider.setValue(slider_val) slider.blockSignals(False) if 0 <= idx < len(self.sim_params['bodies']): self.sim_params['bodies'][idx]['crater_fill'] = value self._updater.request_update() - slider = tab_w.crater_fill_slider # type: ignore - slider.blockSignals(True) - slider.setValue(slider_val) - slider.blockSignals(False) - if 0 <= idx < len(self.sim_params['bodies']): - self.sim_params['bodies'][idx]['crater_fill'] = value - self._updater.request_update()
2304-2304: Unused loop variablerange_val.The
range_valis used for sorting but not within the loop body. Rename to_to indicate it's intentionally unused.♻️ Suggested fix
- for range_val, kind, idx, mask in objects: + for _, kind, idx, mask in objects: if mask is not None and bool(mask[v_i, u_i]):
2393-2395: Replaceassert Falsewithraise AssertionError().Consistent with other instances in this file, use explicit exception raising.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/main/nav_create_simulated_image.pysrc/nav/nav_model/nav_model_rings.pytests/nav/nav_model/test_nav_model_rings_antialiasing.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-14T04:39:08.699Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 80
File: src/nav/nav_model/nav_model_rings.py:873-922
Timestamp: 2026-01-14T04:39:08.699Z
Learning: In nav_model_rings.py, the fade rendering in _render_single_edge intentionally uses edge_radii.filled(0.0) without masking invalid backplane pixels, because the fade calculation handles partial pixel coverage and can legitimately extend into pixels where the backplane center is masked.
Applied to files:
tests/nav/nav_model/test_nav_model_rings_antialiasing.py
📚 Learning: 2025-12-23T19:46:29.815Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 68
File: src/nav/dataset/dataset_pds3_cassini_iss.py:241-241
Timestamp: 2025-12-23T19:46:29.815Z
Learning: In Python, for staticmethod methods, super() cannot be used because there is no class or instance context. If a subclass static method needs to call a parent class's static method, invoke it directly as ParentClass.method_name() rather than super().method_name(). This applies broadly to Python files in the repository; ensure the parent method is defined as a static method or accessible in the class hierarchy and that imports are correct.
Applied to files:
tests/nav/nav_model/test_nav_model_rings_antialiasing.pysrc/nav/nav_model/nav_model_rings.pysrc/main/nav_create_simulated_image.py
📚 Learning: 2026-01-14T04:39:08.699Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 80
File: src/nav/nav_model/nav_model_rings.py:873-922
Timestamp: 2026-01-14T04:39:08.699Z
Learning: In nav_model_rings.py, the fade rendering in _render_single_edge intentionally uses edge_radii.filled(0.0) without masking invalid backplane pixels because the fade calculation handles partial pixel coverage and can legitimately extend into masked areas. Reviewers should preserve this behavior where the fade math can spill over and ensure comments/docs justify it; verify tests cover this behavior and avoid masking in similar edge cases unless the fade logic is changed.
Applied to files:
src/nav/nav_model/nav_model_rings.py
🧬 Code graph analysis (3)
tests/nav/nav_model/test_nav_model_rings_antialiasing.py (1)
src/nav/nav_model/nav_model_rings_base.py (1)
_compute_antialiasing(32-70)
src/nav/nav_model/nav_model_rings.py (6)
src/nav/config/config.py (1)
rings(192-196)src/nav/support/time.py (2)
now_dt(17-24)utc_to_et(56-68)src/nav/nav_model/nav_model_rings_base.py (2)
_create_edge_annotations(72-222)_compute_antialiasing(32-70)src/nav/nav_model/nav_model.py (2)
name(79-81)model_mask(99-101)src/nav/nav_master/nav_master.py (1)
obs(106-108)src/nav/obs/obs_snapshot.py (2)
closest_planet(302-303)ext_bp(341-355)
src/main/nav_create_simulated_image.py (2)
src/nav/obs/obs_snapshot.py (1)
closest_planet(302-303)src/nav/config/config.py (2)
bodies(185-189)rings(192-196)
🪛 Ruff (0.14.11)
tests/nav/nav_model/test_nav_model_rings_antialiasing.py
16-16: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
src/nav/nav_model/nav_model_rings.py
87-87: Boolean-typed positional argument in function definition
(FBT001)
88-88: Boolean-typed positional argument in function definition
(FBT001)
89-89: Boolean-typed positional argument in function definition
(FBT001)
112-112: Logging statement uses f-string
(G004)
120-120: Avoid specifying long messages outside the exception class
(TRY003)
125-125: Avoid specifying long messages outside the exception class
(TRY003)
129-129: Avoid specifying long messages outside the exception class
(TRY003)
131-132: Logging statement uses f-string
(G004)
151-151: Logging statement uses f-string
(G004)
166-166: Logging statement uses f-string
(G004)
281-281: Boolean positional value in function call
(FBT003)
293-293: Boolean positional value in function call
(FBT003)
345-345: Do not catch blind exception: Exception
(BLE001)
347-348: Logging statement uses f-string
(G004)
356-356: Do not catch blind exception: Exception
(BLE001)
358-359: Logging statement uses f-string
(G004)
374-374: Logging statement uses f-string
(G004)
382-382: Logging statement uses f-string
(G004)
422-422: Logging statement uses f-string
(G004)
428-428: Logging statement uses f-string
(G004)
434-434: Logging statement uses f-string
(G004)
454-455: Logging statement uses f-string
(G004)
598-598: Boolean positional value in function call
(FBT003)
650-651: Logging statement uses f-string
(G004)
655-656: Logging statement uses f-string
(G004)
783-783: Logging statement uses f-string
(G004)
794-794: Logging statement uses f-string
(G004)
798-799: Logging statement uses f-string
(G004)
804-804: Boolean positional value in function call
(FBT003)
869-870: Logging statement uses f-string
(G004)
878-879: Logging statement uses f-string
(G004)
896-896: Logging statement uses f-string
(G004)
904-905: Logging statement uses f-string
(G004)
src/main/nav_create_simulated_image.py
1009-1009: Avoid specifying long messages outside the exception class
(TRY003)
1060-1060: Avoid specifying long messages outside the exception class
(TRY003)
1767-1767: Boolean positional value in function call
(FBT003)
1769-1769: Boolean positional value in function call
(FBT003)
1781-1781: Boolean positional value in function call
(FBT003)
1783-1783: Boolean positional value in function call
(FBT003)
1788-1788: Boolean positional value in function call
(FBT003)
1790-1790: Boolean positional value in function call
(FBT003)
1802-1802: Boolean positional value in function call
(FBT003)
1804-1804: Boolean positional value in function call
(FBT003)
1816-1816: Boolean positional value in function call
(FBT003)
1818-1818: Boolean positional value in function call
(FBT003)
1823-1823: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
1881-1881: Boolean-typed positional argument in function definition
(FBT001)
1913-1913: Boolean positional value in function call
(FBT003)
1915-1915: Boolean positional value in function call
(FBT003)
1948-1948: Boolean-typed positional argument in function definition
(FBT001)
1980-1980: Boolean positional value in function call
(FBT003)
1982-1982: Boolean positional value in function call
(FBT003)
2036-2036: Boolean positional value in function call
(FBT003)
2038-2038: Boolean positional value in function call
(FBT003)
2042-2042: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
2270-2270: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
2271-2271: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
2394-2394: Avoid specifying long messages outside the exception class
(TRY003)
2538-2538: Dynamically typed expressions (typing.Any) are disallowed in state
(ANN401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test nav (ubuntu-latest, 3.11)
- GitHub Check: Test nav (ubuntu-latest, 3.10)
- GitHub Check: Test nav (ubuntu-latest, 3.12)
- GitHub Check: Test nav (ubuntu-latest, 3.13)
🔇 Additional comments (15)
src/nav/nav_model/nav_model_rings.py (5)
1-50: LGTM: Imports and class initialization are well-structured.The module docstring clearly describes the purpose, the imports are appropriate, and the class inherits correctly from
NavModelRingsBase. The constructor properly delegates to the superclass.
52-84: LGTM: Public entry point with proper metadata tracking.The
create_modelmethod correctly initializes metadata with timing, handles the logger context manager, and computes elapsed time. The parameter naming is clear (always_create_model,never_create_model,create_annotations).
607-750: LGTM: Edge fade computation with conflict-aware width adjustment.The
_compute_edge_fademethod implements a sophisticated pixel-integrated fade calculation with proper handling of four coverage cases. The conflict detection that adjusts fade width when features are close together is a nice touch for avoiding visual artifacts.
752-831: LGTM: Full ringlet rendering with proper anti-aliasing.The
_render_full_ringletmethod correctly computes both edge radii, fills the solid region between edges, and applies anti-aliasing at the boundaries. The mask handling at lines 811-814 properly accounts for both masked arrays and regular arrays.
832-922: LGTM: Single edge rendering follows the established pattern.This method correctly determines fade direction based on feature type and edge type, calculates fade width from resolution, and delegates to
_compute_edge_fade. The intentional use ofedge_radii.filled(0.0)without masking is appropriate here per the learning that fade calculations handle partial pixel coverage.src/main/nav_create_simulated_image.py (6)
104-128: LGTM: Class rename and expanded sim_params with ring support.The rename to
CreateSimulatedImageModelbetter reflects the broader scope. The new parameters (closest_planet,time,ring_epoch,shade_solid_rings,rings) are well-structured and provide the necessary data model for ring simulation.
1367-1592: LGTM: Ring tab construction is comprehensive.The
_build_ring_tabmethod properly creates UI elements for all ring parameters including:
- Feature type selection (RINGLET/GAP)
- Center coordinates with spinboxes
- Inner/outer edge enable checkboxes with associated mode 1 parameters
- Proper widget property storage for later reference
The checkbox-based enable/disable pattern for inner/outer edges with validation is well-designed.
1881-1946: LGTM: Inner edge enable/disable handler with proper validation.The
_on_ring_inner_enabledmethod correctly:
- Prevents disabling inner when outer is also disabled (must have at least one edge)
- Re-enables the checkbox with signal blocking to prevent recursion
- Reads current UI values when re-enabling instead of using defaults
- Updates UI control states appropriately
2262-2331: LGTM: Selection logic properly handles bodies, rings, and stars with range ordering.The
_select_model_atmethod correctly:
- Collects both bodies and rings with their range values
- Sorts by range to select the topmost (nearest) object first
- Falls back to stars only after checking bodies and rings
- Properly switches to the corresponding tab on selection
2359-2375: LGTM: Ring drag/move support properly implemented.The
_move_selected_bymethod correctly handles rings alongside bodies and stars, updating center coordinates and syncing the tab spinboxes. The pattern matches the existing body and star handling.
2457-2479: LGTM: Load parameters properly handles new ring-related fields.The loading logic correctly extracts
time,ring_epoch,closest_planet, andringsfrom the JSON file and updates the corresponding UI controls.tests/nav/nav_model/test_nav_model_rings_antialiasing.py (4)
1-32: LGTM: Well-structured test setup for anti-aliasing functionality.The MockObservation provides the minimal interface needed for instantiating NavModelRings, and the pytest fixture creates a clean instance for each test. The test organization is clear and follows pytest conventions.
35-87: LGTM: Core anti-aliasing tests cover key edge cases.The tests verify:
- Pixel center at edge → 0.5 shade (correct half-coverage)
- Edge offset by ±0.5 resolution → 1.0 shade (full coverage)
shade_above=True/Falsebehavior with correct per-pixel expectationsThe inline comments explaining the expected calculations are helpful for understanding the test logic.
110-142: LGTM: Value range test with comprehensive validation.The test replicates the implementation logic to compute expected values, then validates each element individually. This ensures the implementation correctly clamps values to [0, 1] across a range of input radii.
157-178: LGTM: 2D array input test validates broadcasting behavior.This test verifies that the anti-aliasing computation correctly handles 2D array inputs, which is important for the actual ring model use case where radii and resolutions are 2D image arrays.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/main/nav_create_simulated_image.py`:
- Around line 2261-2267: The code casts already-integer sim_params entries to
int unnecessarily; update the block in nav_create_simulated_image.py by removing
the int() wraps around self.sim_params['size_v'] and self.sim_params['size_u']
(i.e., set height = self.sim_params['size_v'] and width =
self.sim_params['size_u']) while leaving v_i = int(round(img_v)) and u_i =
int(round(img_u)) as-is, and keep the bounds check and the
self._selected_model_key reset/return logic unchanged.
- Around line 2008-2034: The _validate_ring_edges method is unused dead code;
either delete it or consolidate its logic into the existing handlers
(_on_ring_inner_enabled and _on_ring_outer_enabled). If deleting, remove the
_validate_ring_edges definition and any related imports/comments and run a grep
to confirm no references remain. If consolidating, move the re-enable-inner
logic and UI updates (tab lookup, inner_checkbox signaling, and inner_controls
enabling) into both _on_ring_inner_enabled and _on_ring_outer_enabled so
behavior is identical, and remove the unused method thereafter.
♻️ Duplicate comments (1)
src/main/nav_create_simulated_image.py (1)
261-274: Consider adding an explicit empty/None option for closest_planet.The combo box doesn't have an empty option, so if a user wants to clear the closest planet selection, they must manually delete the text. While the
_on_closest_planethandler (lines 607-613) correctly storesNonefor empty text, the UI doesn't provide an explicit way to select "no planet."Consider adding an empty entry as the first option if
Noneis a valid value:♻️ Suggested improvement
self._closest_planet_combo = QComboBox() self._closest_planet_combo.setEditable(True) - self._closest_planet_combo.addItems(['MERCURY', 'VENUS', 'EARTH', 'MARS', 'JUPITER', + self._closest_planet_combo.addItems(['', 'MERCURY', 'VENUS', 'EARTH', 'MARS', 'JUPITER', 'SATURN', 'URANUS', 'NEPTUNE', 'PLUTO'])
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/main/nav_create_simulated_image.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-23T19:46:29.815Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 68
File: src/nav/dataset/dataset_pds3_cassini_iss.py:241-241
Timestamp: 2025-12-23T19:46:29.815Z
Learning: In Python, for staticmethod methods, super() cannot be used because there is no class or instance context. If a subclass static method needs to call a parent class's static method, invoke it directly as ParentClass.method_name() rather than super().method_name(). This applies broadly to Python files in the repository; ensure the parent method is defined as a static method or accessible in the class hierarchy and that imports are correct.
Applied to files:
src/main/nav_create_simulated_image.py
🧬 Code graph analysis (1)
src/main/nav_create_simulated_image.py (2)
src/nav/obs/obs_snapshot.py (1)
closest_planet(302-303)src/nav/config/config.py (2)
bodies(185-189)rings(192-196)
🪛 Ruff (0.14.11)
src/main/nav_create_simulated_image.py
1009-1009: Avoid specifying long messages outside the exception class
(TRY003)
1060-1060: Avoid specifying long messages outside the exception class
(TRY003)
1767-1767: Boolean positional value in function call
(FBT003)
1769-1769: Boolean positional value in function call
(FBT003)
1781-1781: Boolean positional value in function call
(FBT003)
1783-1783: Boolean positional value in function call
(FBT003)
1795-1795: Boolean positional value in function call
(FBT003)
1797-1797: Boolean positional value in function call
(FBT003)
1809-1809: Boolean positional value in function call
(FBT003)
1811-1811: Boolean positional value in function call
(FBT003)
1816-1816: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
1874-1874: Boolean-typed positional argument in function definition
(FBT001)
1906-1906: Boolean positional value in function call
(FBT003)
1908-1908: Boolean positional value in function call
(FBT003)
1941-1941: Boolean-typed positional argument in function definition
(FBT001)
1973-1973: Boolean positional value in function call
(FBT003)
1975-1975: Boolean positional value in function call
(FBT003)
2029-2029: Boolean positional value in function call
(FBT003)
2031-2031: Boolean positional value in function call
(FBT003)
2035-2035: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
2263-2263: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
2264-2264: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
2387-2387: Avoid specifying long messages outside the exception class
(TRY003)
2531-2531: Dynamically typed expressions (typing.Any) are disallowed in state
(ANN401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test nav (ubuntu-latest, 3.13)
🔇 Additional comments (8)
src/main/nav_create_simulated_image.py (8)
104-128: LGTM! Class rename and data model expansion are well-structured.The class rename from
CreateSimulatedBodyModeltoCreateSimulatedImageModelbetter reflects the broader scope, and thesim_paramsdictionary correctly includes all new ring-related fields (closest_planet,time,ring_epoch,shade_solid_rings,rings).
607-621: LGTM! New parameter handlers follow established patterns.The
_on_closest_planet,_on_time, and_on_epochhandlers correctly updatesim_paramsand trigger updates via_updater.request_update().
884-922: LGTM! Ring tab creation follows established patterns.The
_add_ring_tabmethod correctly:
- Generates unique names using
_find_unique_name- Assigns unique range values
- Initializes both inner and outer edge data with mode 1
- Rebuilds tabs and triggers updates
1367-1433: LGTM! Ring tab UI structure is well-organized.The ring tab correctly includes all necessary controls: name, feature type (RINGLET/GAP), center coordinates, range, and shading distance. The widget properties (
kind,data_index) are set correctly for tab management.
1434-1507: LGTM! Inner edge controls with enable/disable logic are well-implemented.The pattern of storing both the controls list (for batch enable/disable) and individual spinbox references (for reading current values on re-enable) is a good approach.
2255-2303: LGTM! Ring selection integrates well with range-based ordering.The
_select_model_atmethod correctly:
- Collects both bodies and rings with their ranges
- Sorts by range (near to far) for proper occlusion handling
- Uses
_for the unusedrange_valin the loop iteration (addressing previous review feedback)
2450-2472: LGTM! Load/save correctly handles ring parameters.The loading logic properly:
- Includes
time,ring_epoch,closest_planet, andringsin the loaded params- Syncs UI controls for time and epoch spinboxes
- Handles closest_planet combo with both index and custom text cases
2531-2541: LGTM! Shade solid rings toggle and main function are correct.The toggle handler properly updates
sim_params['shade_solid_rings']and triggers a re-render. Themain()function correctly instantiates the renamedCreateSimulatedImageModelclass.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Implement ring models for both real and simulated images.
NavModelRingsintoNavModelBase,NavModelRings, andNavModelRingsSimulatedto mirror body models.compute_ring_modelsinnav_master.py.nav_model_rings.py.NavModelBase.nav_create_simulated_image.py.nav_model_rings_simulated.py,obs_inst_sim.py,render.py, andsim_ring.py.Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.