Refactor planetary ring navigation model into nav.nav_model.rings subpackage#89
Merged
rfrenchseti merged 11 commits intomainfrom Apr 12, 2026
Merged
Refactor planetary ring navigation model into nav.nav_model.rings subpackage#89rfrenchseti merged 11 commits intomainfrom
nav.nav_model.rings subpackage#89rfrenchseti merged 11 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces a comprehensive ring domain model refactor under Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #89 +/- ##
==========================================
+ Coverage 33.56% 40.76% +7.19%
==========================================
Files 62 70 +8
Lines 5541 6229 +688
Branches 715 875 +160
==========================================
+ Hits 1860 2539 +679
+ Misses 3576 3500 -76
- Partials 105 190 +85 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 35
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.cursor/rules/bug_report.mdc:
- Around line 49-50: Replace the vague placeholder "(Example)" under the "2.
Run:" step with a minimal, script-shaped reproducer so reporters provide an
actionable run command; specifically update the content after the "2. Run:"
heading to include a short code block showing e.g. a Python snippet that imports
the target API (replace reponame and target_api with real names) and calls it
(e.g., "from reponame import target_api" followed by "target_api(...)") so the
step is an explicit runnable command rather than a prose placeholder.
In @.cursor/rules/documentation.mdc:
- Around line 13-17: The documentation rule in .cursor/rules/documentation.mdc
forbids unicode but is contradicted by existing files (e.g.,
.cursor/skills/python-codebase-analysis/reference.md uses an en dash and a ≥
symbol); update the rule or the docs so they are consistent: either remove or
relax the blanket prohibition in documentation.mdc to explicitly allow specific
characters (en dash, ≥) and mention lint enforcement, or normalize and replace
unicode in the referenced file(s) (e.g.,
.cursor/skills/python-codebase-analysis/reference.md) to ASCII equivalents so
the rule holds; ensure the change is applied consistently and remove any
stale/contradictory text so the standard and the repo content match.
In @.cursor/rules/how_to.mdc:
- Line 73: The rule text currently permits MkDocs and “similar” renderers;
update the "**Format**: Prefer Mermaid diagrams..." sentence to explicitly
require Sphinx-based rendering and ReadTheDocs hosting by removing references to
MkDocs/similar and replacing them with Sphinx-specific guidance (e.g., mention
sphinxcontrib-mermaid and that documentation is built with Sphinx/ReadTheDocs),
so the line enforces the repo standard and avoids ambiguity.
In @.cursor/skills/critique-test-suite/SKILL.md:
- Line 65: Update the guidance that currently mandates choosing a single
patching approach so it instead allows using either monkeypatch or mock.patch
depending on intent: keep monkeypatch for simple state changes (env vars,
sys.path, module attribute swaps) and prefer mock.patch when you need
call-tracking, assertions, or complex patch lifecycles; remove the statement
that mixing them is an anti-pattern and add a short note recommending
consistency within a single test when it improves readability. Reference the
terms "monkeypatch" and "mock.patch" in the revised text and replace the
existing one-line enforcement with the conditional-usage guidance described
above.
- Line 138: Update the rule text for "No logic in tests" (line referencing the
rule at section 9 / the bullet currently reading "**No logic in tests:** Tests
should not contain conditionals (`if`), loops, or complex logic; note tests that
do and suggest splitting or parameterizing.") to relax the blanket prohibition:
state that simple table-driven tests and straightforward loops/parameterization
are acceptable and recommended, but opaque or complex control flow that obscures
intent is discouraged; keep guidance to split or parameterize tests when logic
makes intent unclear and give examples of allowed patterns (table-driven tests,
simple iteration) vs disallowed patterns (nested conditionals, hidden
side-effects).
- Line 277: Update step 1 to audit all pytest plugin sources: in addition to
reading pyproject.toml, also parse any PYTEST_PLUGINS environment variable and
inspect pytest_plugins declarations in each conftest.py under tests/, and
continue listing all test files and conftest.py; reference the step name "Step
1" and the symbols pyproject.toml, PYTEST_PLUGINS, pytest_plugins, tests/, and
conftest.py so the guidance covers entry-point, env-var, and conftest-specified
plugins.
In @.cursor/skills/run-all-checks/SKILL.md:
- Line 19: The wording in SKILL.md currently conflates activating a virtual
environment with installing dev extras; update the instructions to show two
distinct steps: (1) create and activate an isolated venv (e.g., "python -m venv
venv" then "source venv/bin/activate"), and (2) with the venv active, run
dependency installation like pip install -e ".[dev]"; rephrase the sentence on
Line 19 so it does not imply pip install is an alternative to activating the
venv and add a brief note advising to never install project dependencies into
the system Python.
- Around line 61-68: Add the missing -p/--parallel flag to the Options list in
the run-all-checks SKILL documentation: insert an entry describing that -p,
--parallel runs code checks and docs in parallel (the inverse of
-s/--sequential), matching the behavior exposed by the run-all-checks script so
the documented CLI options (-c/--code, -d/--docs, -m/--markdown,
-s/--sequential, -h/--help) are complete and consistent with the script.
In `@src/nav/config_files/config_01_settings.yaml`:
- Around line 18-19: Change the shipped default log level from DEBUG to INFO by
updating the log_level_model_rings setting in config_01_settings.yaml to "INFO",
and add a short comment noting that DEBUG should be used only in local/dev
override files or via environment-specific configs so production retains lower
verbosity; ensure any deployment/config-management templates or README mention
using DEBUG in local overrides rather than the shipped default.
In `@src/nav/nav_model/nav_model_rings.py`:
- Around line 266-278: Add a short explanatory docstring or inline comment to
the min_res_at_radius closure explaining its purpose and algorithm: that it
returns the minimum radial resolution (km/pixel) at a given radius a by
computing the border mask via obs.ext_bp.border_atop(bp_radii.key, a), selecting
corresponding entries from resolutions, handling empty or fully-masked arrays,
and returning the positive minimum or None; place this docstring at the top of
the min_res_at_radius function so readers of that closure (and future
maintainers) immediately understand the border selection and masking checks.
In `@src/nav/nav_model/rings/ring_feature.py`:
- Around line 674-702: The code treats any entry with an 'a' field as a
base-orbit even if mode_num != 1; update the parsing in the block that
constructs RingBaseOrbitMode (use variables mode_num, mode, feature_key,
edge_type, i) to validate that mode_num == 1 before constructing a base orbit
and raise a ValueError (or at minimum log/warn) when 'a' is present but mode_num
is not 1, including a clear message that the mode value is inconsistent with
base-orbit data; ensure this check happens after extracting mode_num and the
numeric fields but before creating RingBaseOrbitMode.
In `@src/nav/nav_model/rings/ring_math.py`:
- Around line 145-149: compute_fade_integral currently divides by the raw
resolutions which can be zero; before computing result clamp/validate
resolutions just like width (e.g., resolutions_clamped =
np.maximum(np.asarray(resolutions, dtype=np.float64), _FLOAT64_MIN_POS)) and use
resolutions_clamped in the division instead of raw resolutions (or raise a clear
ValueError for invalid zeros at the API boundary); update the division in the
result calculation to reference resolutions_clamped and ensure the dtype stays
float64.
- Around line 152-161: The compute_edge_fade function must validate all inputs
before performing vectorized math: convert inputs to arrays via np.asarray,
assert model and radii are 1-D and shape-compatible (e.g., model.shape ==
radii.shape or otherwise broadcastable), check resolutions and all_edge_radii
lengths match expected dimensions, ensure fade_width_pix is finite and
non-negative (and >0 if zero is invalid for the algorithm), and verify all
numeric arrays contain only finite values (no NaN/Inf) and contain floats; on
any violation raise a clear ValueError mentioning the offending parameter. Use
numpy.isfinite checks and descriptive error messages so failures occur at the
API boundary rather than deep inside compute_edge_fade (apply the same
validation pattern to other nearby functions that perform similar vector math).
In `@src/nav/nav_model/rings/ring_render_context.py`:
- Around line 67-72: Add input validation inside
RingsRenderContext.__post_init__: check obs is not None, ring_target is a
non-empty str, epoch is a finite number (float/int), resolutions is a 1-D
numeric NDArray of positive values, fade_width_pix is a non-negative finite
float, and all_edge_radii is a non-empty tuple of tuples where each inner tuple
is (float radius > 0, non-empty str label). Raise ValueError (or TypeError where
appropriate) with clear messages identifying the offending field (obs,
ring_target, epoch, resolutions, fade_width_pix, all_edge_radii) so invalid
inputs are rejected at the public boundary before rendering internals run.
In `@src/nav/nav_model/rings/ring_render_result.py`:
- Around line 44-47: RingRenderResult should validate its invariants at
construction: add a __post_init__ on the RingRenderResult dataclass to (1) check
uncertainty is a float in the expected range (e.g. 0.0–1.0) and raise ValueError
if not, (2) assert model_img and model_mask have compatible spatial shapes (e.g.
model_img.shape[:2] == model_mask.shape[:2]) and matching ndim expectations, and
(3) validate each entry of edge_info_list: ensure the first element is an
ndarray with boolean dtype and the same spatial shape as model_mask and the
other tuple elements are strings; raise clear ValueError messages referencing
RingRenderResult, model_img, model_mask, uncertainty, and edge_info_list when
checks fail.
In `@src/nav/nav_model/rings/ring_types.py`:
- Around line 76-83: RingBaseOrbitMode.__post_init__ currently only checks
ranges for a, ae, and rms but doesn't validate types, so non-numeric or boolean
values can slip through; update __post_init__ to first verify each of a, ae, rms
is a real numeric (e.g., isinstance(value, numbers.Real) and not
isinstance(value, bool) or equivalent) and raise TypeError with a clear message
like "RingBaseOrbitMode.<field> must be a real number, got <type>" for invalid
types, then proceed with the existing range checks (a > 0, ae >= 0, rms >= 0);
import numbers if needed and reference the class name RingBaseOrbitMode and
method __post_init__ when making the changes.
- Around line 118-130: The constructor currently validates mode_num and
amplitude but not phase or pattern_speed, which later get passed to
math.radians() and can fail in parsed_modes_for_backplane; update
RingPerturbationMode.__post_init__ to validate both phase and pattern_speed:
ensure they are not bool and are instances of (int, float), and also check they
are finite (use math.isfinite) and raise ValueError with descriptive messages
(e.g., "RingPerturbationMode.phase must be a finite number, got ...") to fail
fast and keep parsed_modes_for_backplane safe.
In `@src/nav/ui/common.py`:
- Around line 188-202: Update the public docstring for build_stretch_controls to
fully document all input parameters (including the omitted existing inputs like
min_value, max_value, initial values, orientation or any boolean flags present),
their types and valid ranges, and to add a Google-style Returns: section that
clearly lists the returned mapping keys (slider_black, label_black,
slider_white, label_white, slider_gamma, label_gamma, to_slider, from_slider,
set_values) with their types and behaviors (e.g., QSlider/QLabel widgets and
callable signatures for to_slider(val)->int, from_slider(pos)->float,
set_values(black, white, gamma) -> None), and ensure the Raises: section
mentions exceptions for invalid argument types/values; keep language precise so
callers can write black-box tests against build_stretch_controls.
- Around line 203-214: The current input validation for value_label_min_width
and slider_horizontal_stretch accepts booleans because isinstance(True, int) is
True; update the checks in the public API so booleans are rejected explicitly:
add a guard that checks if type(value_label_min_width) is bool (or
isinstance(..., bool)) and raise TypeError with a clear message before the
int/type and range checks, and do the same for slider_horizontal_stretch (check
for bool then proceed to existing int and range validations).
In `@src/nav/ui/manual_nav_dialog.py`:
- Around line 506-519: _on_reset_model_stretch currently recomputes
_model_black/_model_white and calls
self._model_stretch_controls['set_values'](...) but does not update the slider's
stored min/max mapping, leaving the slider scale stale; after computing
self._model_black and self._model_white, update the control's range (e.g., call
the appropriate method on self._model_stretch_controls such as 'set_range' or
update its min/max entries) so the slider mapping is reset to the new
black/white bounds before calling 'set_values', ensuring subsequent slider moves
correspond to the new slice.
In `@src/pds4/templates/cassini_iss_saturn_1.0/data_orig2.lblx`:
- Line 188: The Discipline_Area currently declares the rings namespace but lacks
the required rings geometry block (see the TODO comment); add a dedicated rings
geometry section inside Discipline_Area that defines ring-specific metadata such
as ring_plane_intercept_points, ring_radii (inner/outer), ring_longitudes
(node/ascending), and optional perturbation_parameters, ensuring each element
uses the declared rings namespace; update any schema references or examples in
the template (e.g., where rings: is declared) so the rings geometry fields are
present and populated or documented as optional placeholders for ring navigation
products.
- Line 250: Replace the TODO in the template with a concrete description that
the supplemental file is a JSON file holding navigation metadata and SPICE
parameters (constructed via json_as_string(combined_metadata)), and either embed
that human-readable description or add a formal Data_File_Object element that
documents the JSON schema/fields (e.g., navigation timestamps, ephemeris IDs,
instrument pointing, and SPICE kernels) so consumers know the expected keys and
types; reference json_as_string(combined_metadata) as the source of contents and
ensure the label clearly states the file name, MIME/type (application/json), and
that it contains navigation/SPICE metadata.
- Line 237: The File_Area_Observational currently only lists a File element but
omits the required Array_3D description for the backplane FITS; add an Array_3D
(or appropriately named Array) child under File_Area_Observational for the FITS
backplane that defines the three axes (e.g., Axis_Array with three Axis entries
or Axis_Samples), Axis_Name values (e.g., SAMPLE, LINE, BAND/LAYER),
dimensions/sizes matching the FITS HDU, Data_Type (e.g., IEEE_REAL or unsigned
integer), Byte_Order, and Element_Attributes (including Scale, Offset, Units if
applicable), and include a concise description of each backplane layer in the
Array_3D/Label_Reference (or Band_Information) so each layer’s semantic meaning
is recorded; update the File element’s Reference_File/Table_External_Description
fields to point to this Array_3D and ensure identifiers match between File and
Array_3D (use existing symbols File_Area_Observational and File) so the label
becomes PDS4-compliant.
- Line 1: Remove or document the unused template file data_orig2.lblx: either
delete data_orig2.lblx if it is an accidental backup, or add documentation
explaining its purpose (e.g., note in a README or a comment in the templates
directory) or explicitly exclude it via .gitignore; ensure bundle_data.py (which
currently loads data.lblx) is unchanged unless you intentionally want to
reference data_orig2.lblx, in which case update bundle_data.py to load that file
instead and document the reason for the change.
- Line 36: Replace the hardcoded <publication_year>2007</publication_year> value
with the correct archival year or make it a template variable; specifically,
change the publication_year element to use a populated token like
$PUBLICATION_YEAR$ (or set it to the actual archival year for this product) so
the derived-product publication_year reflects when the product was
certified/archived per PDS4 DD_1J00 rather than the original observation year
2007; update any generation pipeline/template substitution (the publication_year
tag and template producing this lblx) to ensure $PUBLICATION_YEAR$ is filled at
build time.
- Line 93: The template currently leaves a TODO where Target_Identification
elements must be emitted inside the Observation_Area; implement a
Target_Identification section that iterates over the populated template
variables (TARGET_LIST, TARGET_NAME, TARGET_DESC) and emits one
Target_Identification element per target with the proper PDS4 tags
(Target_Identification, Target_Name, Target_Description) and include LID/LIDVID
references where available (use the stored LID/LIDVID template variable if
present) so the previously collected TARGET_* variables are actually rendered
into the template at the TODO location.
- Line 63: Replace the TODO comment by inserting a Science_Facets element inside
the Primary_Result_Summary element: add child tags for domain (set to "Rings"),
discipline_name (set to "Imaging"), and wavelength_range (populate using the
filter wavelength values from this product by computing the min–max wavelength
and formatting as a range), ensuring the new Science_Facets block follows PDS4
element naming and structure and validates against the PDS4 schema; locate where
Primary_Result_Summary appears and add the Science_Facets element there so the
template generates the facets for this Cassini ISS backplanes product.
- Around line 176-185: The template leaves a hardcoded kernels.ker placeholder
and a TODO in the geom:SPICE_Kernel_Identification block (seen in data.lblx /
data_orig2.lblx) without a clear population strategy; replace the placeholder
and remove the TODO by implementing a deterministic rule: when a bundle-level
metakernel exists, parse and inject only kernels actually used by this product
(or provide a generated per-product metakernel name), otherwise omit kernels and
add a clear geom:comment pointing to the C-matrix in
File_Area_Observational_Supplemental as the authoritative pointing source;
update the template to accept a kernels list variable (e.g., kernellist) or flag
(e.g., use_bundle_metakernel) so automated template rendering can either expand
the kernel filenames or emit the supplemental-C-matrix note instead of the
hardcoded kernels.ker/TODO.
In `@tests/nav/nav_model/test_nav_model_rings.py`:
- Around line 160-168: The test directly constructs NavModelRings via
NavModelRings.__new__ and manually injects internals, which is fragile; instead
create a proper test instance by either mocking or bypassing
NavModelRings.__init__ safely (e.g., patch NavModelRings.__init__ to a no-op in
the test setup) or use a factory that calls the real constructor with
test-friendly dependencies, then set/override the same attributes (_config,
_obs, _models, _metadata, _logger) or inject a MagicMock for the logger;
alternatively use create_autospec/NavModelRings to get a spec'd mock. Ensure you
reference NavModelRings and the attributes
_config/_obs/_models/_metadata/_logger when making the replacement so future
init changes won’t silently break tests.
- Around line 11-19: Reorder the import block so it follows the standard
three-group ordering (standard library, third-party, local) and alphabetize each
group: put "from __future__ import annotations" and "from typing import Any" and
"from unittest.mock import MagicMock, patch" in the standard-library group
(alphabetized), then third-party imports "import numpy as np" and "import
pytest", then the local import "from nav.nav_model.nav_model_rings import
NavModelRings"; you can also run "ruff check --fix" to auto-apply the sorting.
In `@tests/nav/nav_model/test_ring_feature.py`:
- Around line 11-24: Sort and clean the import block in
tests/nav/nav_model/test_ring_feature.py: keep the from __future__ import, then
standard library imports (typing, unittest.mock.MagicMock), then third-party
(numpy as np, pytest), then local project imports (RingFeature,
validate_no_date_overlaps, RingsRenderContext, RingFeatureType, utc_to_et);
remove the unused imports math, unittest.mock.patch, and RingEdgeData so the
file passes I001/F401 CI checks.
In `@tests/nav/nav_model/test_ring_render_context.py`:
- Around line 129-132: The test name/docstring claims it checks the default when
omitted, but _make_result currently supplies edge_info_list=[]; update the test
to actually omit the argument (call _make_result without providing
edge_info_list so RingRenderResult's default is exercised) or, if the intent was
to assert behavior when an empty list is explicitly provided, rename the test
and docstring to test_result_empty_edge_info_list and reflect that it passes
edge_info_list=[], referencing the factory _make_result and the
RingRenderResult.edge_info_list attribute so reviewers can locate the change.
In `@tests/nav/nav_model/test_ring_types.py`:
- Around line 11-12: Remove the stale bootstrap comment above the import block
in the test file that references ring_types; specifically delete or update the
line comment preceding the import of nav.nav_model.rings.ring_types (the import
that begins with "from nav.nav_model.rings.ring_types import (") so the file no
longer claims the imports will fail until ring_types.py exists.
- Around line 57-62: The test test_base_orbit_mode_is_frozen currently only
checks the exception type which can mask other attribute errors; update it to
capture the exception with pytest.raises(AttributeError) as exc_info and assert
the exception message contains the frozen-assignment text (e.g. "cannot assign"
or "cannot assign to field 'a'") to ensure the error is the dataclass
FrozenInstanceError; apply the same change pattern to the other immutability
tests mentioned (the tests around the other ranges) that currently only assert
the exception type, referencing RingBaseOrbitMode and the specific assignment
expressions used in those tests when forming the expected message.
- Around line 105-149: Add tests to assert constructor rejects invalid mode_num
types/values and non-numeric amplitude: write a test that calls
RingPerturbationMode with a non-integer/non-numeric mode_num (e.g., "a" or 3.5)
and expects a TypeError/ValueError, and a test that passes a non-numeric
amplitude (e.g., "x") and expects a TypeError/ValueError; use clear names like
test_perturbation_mode_invalid_mode_num and
test_perturbation_mode_non_numeric_amplitude and reference the
RingPerturbationMode constructor to locate the code under test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9a6ae29d-1167-4b2c-adee-44c71feba214
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
.cursor/rules/bug_report.mdc.cursor/rules/documentation.mdc.cursor/rules/environment_best_practices.mdc.cursor/rules/git_workflow.mdc.cursor/rules/how_to.mdc.cursor/rules/javascript_typescript_best_practices.mdc.cursor/rules/pull_request.mdc.cursor/rules/python_best_practices.mdc.cursor/skills/codebase-analysis/reference.md.cursor/skills/critique-test-suite/SKILL.md.cursor/skills/python-codebase-analysis/SKILL.md.cursor/skills/python-codebase-analysis/reference.md.cursor/skills/run-all-checks/SKILL.mddocs/api_reference/api_nav_model.rstdocs/developer_guide_class_hierarchy.rstdocs/developer_guide_configuration.rstdocs/introduction_configuration.rstsrc/nav/config_files/config_01_settings.yamlsrc/nav/config_files/config_20_saturn_rings.yamlsrc/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_model/rings/__init__.pysrc/nav/nav_model/rings/ring_feature.pysrc/nav/nav_model/rings/ring_filter.pysrc/nav/nav_model/rings/ring_math.pysrc/nav/nav_model/rings/ring_render_context.pysrc/nav/nav_model/rings/ring_render_result.pysrc/nav/nav_model/rings/ring_types.pysrc/nav/navigate_image_files.pysrc/nav/ui/common.pysrc/nav/ui/manual_nav_dialog.pysrc/pds4/templates/cassini_iss_saturn_1.0/data_orig2.lblxtests/nav/nav_model/conftest.pytests/nav/nav_model/test_nav_model_rings.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.pytests/nav/nav_model/test_nav_model_rings_simulated.pytests/nav/nav_model/test_ring_feature.pytests/nav/nav_model/test_ring_filter.pytests/nav/nav_model/test_ring_math.pytests/nav/nav_model/test_ring_render_context.pytests/nav/nav_model/test_ring_types.py
💤 Files with no reviewable changes (4)
- .cursor/skills/codebase-analysis/reference.md
- tests/nav/nav_model/test_nav_model_rings_antialiasing.py
- tests/nav/nav_model/test_nav_model_rings_feature_filtering.py
- tests/nav/nav_model/test_nav_model_rings_edge_fade.py
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/pull_request_template.md:
- Line 1: The PR template's top-level heading currently uses "## Purpose" which
triggers MD041; change the first line from "## Purpose" to a level-1 heading "#
Purpose" so the file starts with an H1 (update the string "## Purpose" to "#
Purpose" at the top of the .github/pull_request_template.md).
- Line 7: Rename the PR template heading labeled "## Changes" to the required
"## Changes/Implementation Details" to match repository guidelines; locate the
heading string "## Changes" in the .github pull request template and update it
to "## Changes/Implementation Details" so the template enforces the required
section name exactly.
- Around line 49-50: Update the PR template's Notes section so it is mandatory:
replace the current optional wording ("Delete this section if not needed.") in
the Notes block with a clear instruction to always fill the section and to write
"None" when there are no notes; locate the Notes HTML comment block (the comment
starting "<!-- Anything reviewers should know: tricky areas, unresolved
questions, follow-up work. Delete this section if not needed. -->") and change
its text accordingly to require content.
In `@src/nav/nav_model/nav_model_rings.py`:
- Around line 198-215: The float conversions for fade_width_pix,
min_allowed_fade_width_pix, and min_feature_pixels accept booleans and NaN;
update the validation in nav_model_rings.py to first check the raw values from
planet_config (for keys 'fade_width_pix', 'min_allowed_fade_width_pix',
'min_feature_pixels') reject bools (e.g., isinstance(value, bool)) and ensure
numeric finiteness via math.isfinite before converting/accepting, then raise
ValueError with the same contextual message if the checks fail; apply this to
the existing variables fade_width_pix, min_allowed_fade_width_pix, and
min_feature_pixels.
- Around line 276-285: The current guarded check using hasattr(res_at_edge,
'is_all_masked') in the block handling border_arr/res_at_edge doesn't detect
fully masked NumPy masked arrays, causing float(np.min(res_at_edge)) to produce
nan; replace that guard with a check using np.ma.count(res_at_edge) == 0 (or
np.ma.count(res_at_edge) to detect zero valid entries) so that when res_at_edge
is empty or fully masked the function returns None; update the logic around
res_at_edge, min_val and the return to use this np.ma.count check (references:
border_arr, res_at_edge, np.min).
In `@src/nav/nav_model/rings/ring_feature.py`:
- Around line 680-701: The current code in ring_feature.py converts YAML values
with int()/float() (e.g., coercing mode_num and numeric fields in the mode dict)
before the typed validators run, which lets booleans slip through; instead stop
coercing raw inputs here—either validate the raw values' types (ensure mode_num
is an int and numeric fields are numbers) or pass the unmodified mode dict and
mode_num into the dataclass constructors / from_config() (e.g.,
RingBaseOrbitMode, RingPerturbationMode, from_config()) so their public
validators enforce types; apply the same change to the other similar block
(lines referenced 717-725) where numeric coercion occurs.
- Around line 712-714: The code currently overwrites an existing base_orbit when
creating a RingBaseOrbitMode (base_orbit = RingBaseOrbitMode(...)); change this
to validate and reject duplicates by checking if base_orbit is already set for
the current edge before assigning and raise a clear error (e.g., ValueError)
including edge-identifying info and the offending mode record; update the code
path that parses mode records (where base_orbit is assigned) to perform this
check and fail fast instead of silently overwriting, so malformed YAML with
multiple 'a' entries is surfaced at the public API boundary.
In `@src/nav/nav_model/rings/ring_types.py`:
- Around line 135-141: The amplitude validation in RingPerturbationMode
currently only checks type and positivity, allowing NaN/inf; update the
validation in the RingPerturbationMode.__post_init__ (or wherever amplitude is
validated) to reject non-finite values by using math.isfinite(amplitude) (and
ensure amplitude is numeric and not a bool) and raise ValueError if not finite
or < 0, matching the existing checks for phase and pattern_speed; also add
import math if missing and update the error message to reflect non-finite
rejection.
- Around line 79-93: In RingBaseOrbitMode.__post_init__, extend the current
validation (which now checks only a, ae, rms) to validate every numeric
base-orbit field (e.g., long_peri, rate_peri, rate_node, inc, and any other
numeric attributes on RingBaseOrbitMode) by asserting each is an instance of
numbers.Real (and not bool) and is finite (use math.isfinite), rejecting None or
non-numeric values with TypeError; retain and keep the existing semantic range
checks for a (>0), ae (>=0), rms (>=0) and raise ValueError for out-of-range
numeric values so invalid inputs fail fast at construction.
In `@src/nav/ui/manual_nav_dialog.py`:
- Around line 443-446: The _is_checked function should stop using double
int/cast and instead compare the checkbox state robustly to the enum; replace
the current body with a direct enum comparison such as returning
Qt.CheckState(state) == Qt.CheckState.Checked (or alternatively state ==
Qt.CheckState.Checked.value) in the _is_checked method so it clearly and safely
checks against Qt.CheckState.Checked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6d2b48cc-ccd5-46da-b5ea-27f3f0d4c627
📒 Files selected for processing (15)
.github/pull_request_template.mdsrc/nav/nav_model/nav_model_rings.pysrc/nav/nav_model/rings/ring_feature.pysrc/nav/nav_model/rings/ring_math.pysrc/nav/nav_model/rings/ring_render_context.pysrc/nav/nav_model/rings/ring_render_result.pysrc/nav/nav_model/rings/ring_types.pysrc/nav/ui/common.pysrc/nav/ui/manual_nav_dialog.pytests/nav/nav_model/conftest.pytests/nav/nav_model/test_nav_model_rings.pytests/nav/nav_model/test_nav_model_rings_simulated.pytests/nav/nav_model/test_ring_feature.pytests/nav/nav_model/test_ring_render_context.pytests/nav/nav_model/test_ring_types.py
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/nav/sim/sim_body.py (2)
338-345: 🛠️ Refactor suggestion | 🟠 MajorExpand nested helper docstring to required Google style
power_law_radiusis touched but still has a minimal one-line docstring. It should includeParameters,Returns, andRaisesso behavior is testable from docs alone.Proposed fix
def power_law_radius( rng: np.random.RandomState, R_min: float, R_max: float, alpha: float, size: int | None = None, ) -> NDArrayFloatType: - """Sample R from p(R) ~ R^(-alpha) on [R_min, R_max], alpha > 1.""" + """Sample crater radii from a bounded power-law distribution. + + Parameters: + rng: Random number generator used for sampling. + R_min: Minimum radius bound (inclusive), in pixels. + R_max: Maximum radius bound (inclusive), in pixels. + alpha: Power-law exponent. Must be greater than 1. + size: Optional number of samples. If omitted, returns one sample. + + Returns: + One sampled radius or an array of sampled radii in `[R_min, R_max]`. + + Raises: + ValueError: If `alpha <= 1`. + """As per coding guidelines, "
**/*.py: Every class, method, function, and module MUST have a descriptive docstring following PEP 257 in Google style with Parameters, Returns, Raises, and behavioral notes sufficient to write a black-box test."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/sim/sim_body.py` around lines 338 - 345, Update the one-line docstring for the function power_law_radius to a full Google-style docstring: add a Parameters section documenting rng (np.random.RandomState), R_min (float), R_max (float), alpha (float), and size (int | None) with units/constraints; add a Returns section describing the returned NDArrayFloatType shape and value range; add a Raises section documenting ValueError for invalid inputs (e.g., alpha <= 1, R_min <= 0, R_max <= R_min); and include a short Notes/Behavior section that explains it samples from the distribution p(R) ∝ R^(-alpha) on [R_min, R_max] and how size controls output shape (scalar vs. array).
15-33:⚠️ Potential issue | 🟠 MajorAdd argument validation at the public API boundary
create_simulated_bodydocuments input constraints but does not enforce them. Invalid values (e.g., non-positive axes,anti_aliasingoutside[0,1], invalid crater radii) can lead to divide-by-zero, invalid geometry, or unstable behavior.Proposed fix
def create_simulated_body( @@ ) -> NDArrayFloatType: @@ - size_v, size_u = size + size_v, size_u = size + if size_v <= 0 or size_u <= 0: + raise ValueError("size must contain positive dimensions") + if axis1 <= 0 or axis2 <= 0 or axis3 <= 0: + raise ValueError("axis1, axis2, and axis3 must be > 0") + if not (0.0 <= anti_aliasing <= 1.0): + raise ValueError("anti_aliasing must be between 0.0 and 1.0") + if crater_fill < 0.0: + raise ValueError("crater_fill must be >= 0.0") + if crater_min_radius <= 0.0 or crater_max_radius <= 0.0: + raise ValueError("crater radii must be > 0.0") + if crater_min_radius > crater_max_radius: + raise ValueError("crater_min_radius must be <= crater_max_radius") + if crater_power_law_exponent <= 1.0: + raise ValueError("crater_power_law_exponent must be > 1.0") + if crater_relief_scale < 0.0: + raise ValueError("crater_relief_scale must be >= 0.0")As per coding guidelines, "
**/*.py: Validate inputs at the public API boundary of the library. Raise clearValueErrororTypeErrorexceptions for invalid arguments."Also applies to: 70-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/sim/sim_body.py` around lines 15 - 33, Add explicit input validation at the public API boundary inside create_simulated_body: verify size is a 2-tuple of positive ints, center is a 2-tuple of finite numbers, axis1/axis2/axis3 are positive numbers, rotation_z/rotation_tilt/illumination_angle/phase_angle are finite, anti_aliasing is a number in [0,1], crater_min_radius and crater_max_radius are positive with crater_max_radius >= crater_min_radius, crater_power_law_exponent > 0, crater_relief_scale is non-negative (and bound if desired), and seed is int or None; for any violation raise a clear ValueError or TypeError naming the offending parameter so callers get immediate, actionable errors from create_simulated_body.src/nav/ui/common.py (1)
174-187: 🧹 Nitpick | 🔵 TrivialGive
build_stretch_controls()a typed return object.Adding
set_rangemakes this helper’s return surface even broader, butdict[str, Any]still hides every key and callable from mypy. ATypedDictor small dataclass would make the contract explicit and catch string-key typos in callers at type-check time.As per coding guidelines, "Use the Receive-an-Object, Return-an-Object (RORO) pattern when a function takes or returns more than a few related values: accept a dataclass or TypedDict and return one."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/ui/common.py` around lines 174 - 187, The function build_stretch_controls currently returns a loose dict[str, Any], which hides keys and callables (including set_range) from static typing; replace the untyped dict return with a concrete typed return (either a TypedDict or a small dataclass) that enumerates all returned fields and their precise types (e.g., sliders, labels, and the set_range Callable) so callers get proper mypy checks and autocompletion; update build_stretch_controls signature to return that type and adjust any callers to use the named attributes/keys instead of raw dict access.
♻️ Duplicate comments (1)
src/nav/nav_model/rings/ring_feature.py (1)
645-659:⚠️ Potential issue | 🟠 MajorDo not coerce float
modevalues to integers.
_integral_mode_num()still accepts YAML likemode: 2.0and normalizes it to2.
That bypasses the stricter "int only" contract enforced byRingPerturbationModeand
lets malformed config pass throughfrom_config()instead of failing fast. Remove the
float-to-int branch so YAML must provide a real integer here too.As per coding guidelines: "NEVER trust external input (function arguments from callers, file contents, environment variables, data from remote URLs)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/nav_model/rings/ring_feature.py` around lines 645 - 659, _integration: _integral_mode_num currently accepts finite floats equal to their floor and coerces them to int (allowing YAML like mode: 2.0); remove that float-to-int branch so only true ints are accepted: keep the bool check and the isinstance(mode_num, int) return, delete the "if isinstance(mode_num, float) ..." branch, and ensure the final ValueError message (which references feature_key, edge_type, index, mode_num) is raised for any non-int (including floats) to enforce strict int-only input consistent with RingPerturbationMode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.cursor/skills/critique-test-suite/SKILL.md:
- Around line 169-173: The pytest config discovery guidance only mentions
pyproject.toml and pytest.ini; update both places that list pytest config
sources to enumerate the full discovery precedence and clarify that the first
matching file is used: 1) pytest.toml, 2) .pytest.toml, 3) pytest.ini, 4)
.pytest.ini, 5) pyproject.toml (only if it contains [tool.pytest] or
[tool.pytest.ini_options]), 6) tox.ini (only if it contains a [pytest] section),
and 7) setup.cfg (only if it contains a [tool:pytest] section); ensure the text
explicitly states this precedence and that only the first matching config file
is applied.
In @.cursor/skills/python-codebase-analysis/reference.md:
- Around line 69-75: The "Severity phrasing" section is missing the "Trivial"
level; update the list under the "## Severity phrasing" heading to include a
"Trivial" bullet with suggested phrasing such as "Trivial: 'minor cosmetic
issue', 'does not affect functionality', 'purely informational'"; ensure the
taxonomy now matches the project scale (Critical, High, Medium, Low, Trivial)
and keep the phrasing style consistent with the other entries.
- Line 57: Remove the incorrect reference to mypy in the sentence mentioning
line-length; update the sentence that currently reads something like “Ruff is
configured with `line-length = 88` but mypy uses no line-length setting and the
project rule says 100” to instead compare only Ruff and the project rule (e.g.,
“Ruff is configured with `line-length = 88` while the project rule specifies
100”), and ensure the Evidence section only references `pyproject.toml`
`[tool.ruff]` vs `.cursor/rules/python_best_practices.mdc` without mentioning
mypy.
In @.cursor/skills/run-all-checks/SKILL.md:
- Line 75: The fenced code block in SKILL.md that contains the task list (the
"Check Progress:" block) is missing a language identifier; update the opening
fence to declare the language (use "markdown") so the block becomes ```markdown
and preserves the GitHub-flavored Markdown task list rendering and satisfies
linting.
In `@docs/developer_guide_class_hierarchy.rst`:
- Around line 191-234: The Mermaid class hierarchy diagram is missing the new
orbital value types and their relationships; update the diagram to include
RingFeatureType, RingBaseOrbitMode, and RingPerturbationMode and show their
associations with RingEdgeData (which contains base_orbit, perturbations,
base_radius, rms), and ensure RingFeature links to RingEdgeData,
RingFeatureFilter, RingsRenderContext, and RingRenderResult as in the prose;
keep the diagram nodes and arrows consistent with the documented fields (e.g.,
RingEdgeData -> perturbations : RingPerturbationMode[], RingFeature ->
feature_type : RingFeatureType, RingEdgeData -> base_orbit : RingBaseOrbitMode)
so the visual matches the written API surface and contracts.
In `@src/nav/nav_model/nav_model_rings.py`:
- Around line 210-245: The code assumes ring_features_dict[planet] and its
'features' member are dicts and will raise AttributeError if the YAML is
malformed; add explicit type checks: verify ring_features_dict[planet] (the
local variable planet_config) is an instance of dict and raise a ValueError with
a clear message if not, and similarly verify that planet_config.get('features',
{}) returns a dict before using it (raise ValueError if it's present but not a
dict); update the error messages to reference the planet name so failures are
actionable and ensure subsequent uses of planet_config and features_dict (e.g.,
in _positive_finite_planet_scalar and any .items() loops) are protected by these
checks.
In `@src/nav/nav_model/rings/ring_feature.py`:
- Around line 717-725: The code currently defaults missing perturbation fields
to 0.0 in the else branch that builds RingPerturbationMode entries, which masks
YAML mistakes; change the logic in the block that calls _integral_mode_num and
appends to perturbations so it checks that the dict keys 'amplitude', 'phase',
and 'pattern_speed' are present on mode and raises a ValueError (or TypeError)
with a clear message referencing feature_key, edge_type, and mode index if any
are missing, then pass the raw values into RingPerturbationMode (letting its own
validators handle type/range checks) instead of supplying 0.0 defaults.
In `@src/nav/nav_model/rings/ring_math.py`:
- Around line 126-131: Validate that resolutions are strictly positive at the
public API boundary: in the ring_math module, after calling
_as_finite_float64('resolutions', resolutions) and _require_matching_shapes,
replace the current clamping logic that uses res_safe = np.maximum(res,
_FLOAT64_MIN_POS) with an explicit check that raises ValueError if any entry in
the array `res` is <= 0 (this affects compute_antialiasing and compute_edge_fade
callers); likewise, ensure compute_fade_integral explicitly raises ValueError
for a non-positive `width` argument if that function remains public. Use the
existing symbol names (res, res_safe, compute_antialiasing, compute_edge_fade,
compute_fade_integral) to locate where to add the checks and raise clear
ValueError messages instead of silently clamping.
In `@src/nav/nav_model/rings/ring_types.py`:
- Around line 178-196: Add a __post_init__ to RingEdgeData that validates and
normalizes inputs: verify base_orbit is an instance of RingBaseOrbitMode (raise
TypeError if not), ensure perturbations is converted to an immutable tuple (use
object.__setattr__ to assign since the dataclass is frozen) and raise TypeError
if any element is not a RingPerturbationMode, and raise ValueError for a
missing/None base_orbit; include clear error messages mentioning RingEdgeData,
base_orbit, and perturbations so invalid shapes fail immediately at
construction.
In `@src/nav/ui/common.py`:
- Around line 188-256: build_stretch_controls currently validates layout args
but lets non-numeric or non-finite image/gamma values through causing opaque
failures later; add upfront validation for img_min, img_max, black_init,
white_init, gamma_init in build_stretch_controls: for each ensure the value is a
number (int/float but not bool) and math.isfinite(value); if a type is wrong
raise TypeError naming the parameter, and if not finite raise ValueError naming
the parameter; keep existing degenerate-range behavior for img_max <= img_min
(do NOT change to_slider/from_slider logic), just fail fast with clear errors
before any round() or QSlider.setValue() calls.
In `@src/nav/ui/manual_nav_dialog.py`:
- Around line 463-492: Add Google-style docstrings to each UI handler shown
(_on_model_black_changed, _on_model_white_changed, _on_model_gamma_changed,
_on_model_transparency_changed, _on_show_image_changed, _on_show_model_changed)
and to the related _toggle_zoom_sharp() method: each docstring should follow PEP
257 and include a short description, Parameters (type and meaning of inputs like
val or state), Returns (usually None), Raises (if any), and behavioral notes
describing the UI side effects (which internal attributes are updated, which
widgets are enabled/disabled, and that _refresh_overlay() is called). Ensure the
wording is test-friendly (explicit state transitions) so black-box tests can
assert expected side effects.
In `@tests/nav/nav_model/test_nav_model_rings.py`:
- Around line 393-395: The local patch handle MockFilter should use snake_case;
rename each patch binding (e.g., the one patching
'nav.nav_model.nav_model_rings.RingFeatureFilter') from MockFilter to
mock_filter_cls and update its uses (e.g., mock_filter_inst = MagicMock();
mock_filter_cls.return_value = mock_filter_inst). Repeat the same rename for the
other occurrences noted (around the blocks at 418-420, 453-455, 487-494) so all
local variables and patch handles follow lowercase_with_underscores (e.g.,
mock_filter_cls, mock_something_else_cls) and ensure return_value assignments
and any assertions reference the new names.
- Around line 192-505: The test classes and test methods (e.g., class
TestConfigLoading and its methods test_no_closest_planet_returns_early,
test_missing_planet_config_returns_early, etc.; also
TestCrossFeatureValidation.test_overlapping_dated_features_raises,
TestFilterIntegration._make_render_result and its tests,
TestUncertaintyWiring.test_uncertainty_wired_from_render_result,
TestNeverCreateModel.test_never_create_model_no_images) currently use
summary-only docstrings that do not follow the project's required Google-style
docstring format; update each class and test method to include a Google-style
docstring with a one-line summary plus any required sections (Parameters,
Returns, Raises, and behavioral notes) even if empty (use brief "None" or "N/A"
where appropriate) so they comply with the guideline that every
class/method/function/module has a descriptive Google-style docstring.
---
Outside diff comments:
In `@src/nav/sim/sim_body.py`:
- Around line 338-345: Update the one-line docstring for the function
power_law_radius to a full Google-style docstring: add a Parameters section
documenting rng (np.random.RandomState), R_min (float), R_max (float), alpha
(float), and size (int | None) with units/constraints; add a Returns section
describing the returned NDArrayFloatType shape and value range; add a Raises
section documenting ValueError for invalid inputs (e.g., alpha <= 1, R_min <= 0,
R_max <= R_min); and include a short Notes/Behavior section that explains it
samples from the distribution p(R) ∝ R^(-alpha) on [R_min, R_max] and how size
controls output shape (scalar vs. array).
- Around line 15-33: Add explicit input validation at the public API boundary
inside create_simulated_body: verify size is a 2-tuple of positive ints, center
is a 2-tuple of finite numbers, axis1/axis2/axis3 are positive numbers,
rotation_z/rotation_tilt/illumination_angle/phase_angle are finite,
anti_aliasing is a number in [0,1], crater_min_radius and crater_max_radius are
positive with crater_max_radius >= crater_min_radius, crater_power_law_exponent
> 0, crater_relief_scale is non-negative (and bound if desired), and seed is int
or None; for any violation raise a clear ValueError or TypeError naming the
offending parameter so callers get immediate, actionable errors from
create_simulated_body.
In `@src/nav/ui/common.py`:
- Around line 174-187: The function build_stretch_controls currently returns a
loose dict[str, Any], which hides keys and callables (including set_range) from
static typing; replace the untyped dict return with a concrete typed return
(either a TypedDict or a small dataclass) that enumerates all returned fields
and their precise types (e.g., sliders, labels, and the set_range Callable) so
callers get proper mypy checks and autocompletion; update build_stretch_controls
signature to return that type and adjust any callers to use the named
attributes/keys instead of raw dict access.
---
Duplicate comments:
In `@src/nav/nav_model/rings/ring_feature.py`:
- Around line 645-659: _integration: _integral_mode_num currently accepts finite
floats equal to their floor and coerces them to int (allowing YAML like mode:
2.0); remove that float-to-int branch so only true ints are accepted: keep the
bool check and the isinstance(mode_num, int) return, delete the "if
isinstance(mode_num, float) ..." branch, and ensure the final ValueError message
(which references feature_key, edge_type, index, mode_num) is raised for any
non-int (including floats) to enforce strict int-only input consistent with
RingPerturbationMode.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed078670-0e3e-44b1-a69b-743501b499c8
📒 Files selected for processing (25)
.cursor/rules/how_to.mdc.cursor/skills/critique-test-suite/SKILL.md.cursor/skills/python-codebase-analysis/reference.md.cursor/skills/run-all-checks/SKILL.mddocs/developer_guide_class_hierarchy.rstdocs/developer_guide_introduction.rstdocs/user_guide_backplanes.rstdocs/user_guide_navigation.rstdocs/user_guide_pds4_bundle.rstsrc/main/nav_create_simulated_image.pysrc/nav/nav_model/nav_model_rings.pysrc/nav/nav_model/nav_model_rings_base.pysrc/nav/nav_model/rings/ring_feature.pysrc/nav/nav_model/rings/ring_math.pysrc/nav/nav_model/rings/ring_types.pysrc/nav/nav_technique/nav_technique_correlate_all.pysrc/nav/sim/render.pysrc/nav/sim/sim_body.pysrc/nav/support/correlate.pysrc/nav/support/image.pysrc/nav/ui/common.pysrc/nav/ui/manual_nav_dialog.pytests/nav/nav_model/test_nav_model_rings.pytests/nav/nav_model/test_ring_feature.pytests/nav/nav_model/test_ring_types.py
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (1)
src/nav/nav_model/rings/ring_math.py (1)
260-265:⚠️ Potential issue | 🟠 MajorReject
fade_width_pix == 0before entering the integral cases.
compute_edge_fade()currently accepts0.0, but any pixel whose edge lands inside the pixel will reachcompute_fade_integral()withwidth == 0and fail from inside the case logic. Either short-circuit zero as “no fade” or reject it here so the API boundary stays deterministic.As per coding guidelines, "Validate inputs at the public API boundary of the library. Raise clear
ValueErrororTypeErrorexceptions for invalid arguments."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/nav_model/rings/ring_math.py` around lines 260 - 265, The function should reject fade_width_pix == 0 before reaching the integral cases to avoid compute_fade_integral() being called with width == 0; in the validation block (around the checks that coerce fade_width_pix to fwp) add a guard after fwp = float(fade_width_pix) that raises ValueError (with a clear message) when fwp == 0.0 (or alternatively short-circuit in compute_edge_fade to treat 0 as “no fade”); reference compute_edge_fade, compute_fade_integral, and the local variable fwp when locating where to add the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.cursor/rules/documentation.mdc:
- Line 15: Fix the typo in the documentation rule sentence: replace the
misspelled word "acceptible" with the correct "acceptable" in the sentence
starting "They are also acceptible in source code strings..." so the rule reads
"They are also acceptable in source code strings when needed to present special
characters to the user such as degree signs or arrows at runtime." Update the
string in .cursor/rules/documentation.mdc (the rule sentence) accordingly.
In `@docs/developer_guide_navigation_models_rings.rst`:
- Around line 217-218: Update the documentation unit labels for the ring
parameters rate_peri and pattern_speed from "degrees per year" to "degrees per
day" in the docs/developer_guide_navigation_models_rings.rst entries; this
should match the docstrings in ring_types.py (see rate_peri and pattern_speed
docstrings around the existing comments) and the conversion logic that divides
by 86400 to convert degrees/day to radians/second. Also scan for any other
instances of "degrees per year" referring to these parameters and correct them
to "degrees per day" to keep docs consistent with the code.
In `@src/nav/nav_model/nav_model_rings.py`:
- Around line 225-228: The code currently takes epoch_str from planet_config and
calls utc_to_et() without validating type or format; update the block that sets
epoch_str so it first checks that planet_config.get('epoch') is a string
(isinstance(epoch_str, str)) and if not raises a ValueError referencing the
planet and that the epoch must be a string, then call utc_to_et(epoch_str)
inside a try/except that catches parsing errors and re-raises a ValueError with
clear planet-scoped context; refer to the variables epoch_str, planet_config,
planet and the function utc_to_et when making these changes.
In `@src/nav/nav_model/rings/ring_feature.py`:
- Around line 716-733: The perturbation branch currently accepts mode_num == 1
which is reserved for the base orbit; update the validation in the branch that
builds RingPerturbationMode (after computing mode_n via _integral_mode_num and
before appending RingPerturbationMode) to explicitly reject mode_n == 1 by
raising a ValueError with a clear message referencing the feature_key, edge_type
and index i; this prevents parsed_modes_for_backplane() from emitting a (1,
amplitude, phase, speed) tuple that would be wrongly treated as a perturbation
by _compute_edge_radii()/radial_mode().
- Around line 293-305: The current render() branch treats any
RingFeatureType.RINGLET with non-None inner_radii_bp and outer_radii_bp as a
"full ringlet" and calls _render_full_ringlet(), which causes a solid band even
when one edge is off-screen; change the precondition so full-ringlet rendering
only happens when both edges are actually visible (not merely present): update
the if-condition in render() to check explicit edge visibility state (e.g., use
whatever visibility flags or methods you have for the inner and outer edges
instead of just inner_radii_bp/outer_radii_bp), otherwise fall through to the
single-edge fade path; ensure context.logger.debug messages reflect the chosen
branch (keep the debug call near _render_full_ringlet) and add a regression test
that constructs a RingFeature with RingFeatureType.RINGLET where one edge is
off-screen to assert the single-edge fade rendering path is used.
- Around line 568-605: from_config currently assumes `data` is a dict and will
raise AttributeError on non-dict inputs; add an explicit input validation at the
start of `RingFeature.from_config` that checks `isinstance(data, dict)` (and
optionally validate `key` is a str) and raise a clear TypeError or ValueError
(e.g. "Feature {key!r}: data must be a mapping/dict, got {type(data).__name__}")
before any `data.get(...)` calls so subsequent code (e.g. `raw_type =
data.get('feature_type')`, `RingFeatureType(raw_type)`) only runs for valid dict
inputs.
In `@src/nav/nav_model/rings/ring_filter.py`:
- Around line 91-94: Update the function/method docstring that includes numeric
constraint ValueError (the docstring in ring_filter.py for the routine that
validates parameters and checks logger) to explicitly document the additional
ValueError raised when logger is None; add a line under Raises such as
"ValueError: If logger is None" so the docstring reflects the check performed at
the logger validation (the code that raises ValueError for logger being None).
In `@src/nav/nav_model/rings/ring_render_context.py`:
- Around line 116-134: RingsRenderContext.all_edge_radii is documented as sorted
but not validated; after the existing per-item checks for numeric rad, validate
the sequence order by iterating through the radii (rad from each pair) and
ensure each rad is strictly increasing relative to the previous one (or
non-decreasing if the contract allows), raising a ValueError referencing
RingsRenderContext.all_edge_radii and the offending index if the order is wrong;
add this check within the same validation block (using the existing variables
pair, rad, j) so unsorted input fails fast with a clear message.
- Around line 91-101: The constructor currently validates type, dtype, emptiness
and finiteness of self.resolutions but never enforces that it is 2-D; add a
dimensionality check (e.g., after verifying it's an ndarray) to raise a
ValueError if self.resolutions.ndim != 2 so only per-pixel 2-D arrays are
accepted (keep the rest of the checks and the res64 conversion as-is). Target
the validation block that handles self.resolutions in the RingsRenderContext
(the self.resolutions variable) and raise a clear ValueError message like
"RingsRenderContext.resolutions must be a 2-D array".
In `@src/nav/nav_model/rings/ring_types.py`:
- Around line 291-302: The code repeats the literal 86400.0 for seconds-per-day
conversions; declare a module-level constant (e.g., SECONDS_PER_DAY = 86400.0)
at the top of ring_types.py and replace the hardcoded 86400.0 usages in the
block that converts base_orbit.rate_peri and in the radial_perturbations loop
(references: base_orbit.rate_peri, radial_perturbations(), p.pattern_speed) to
use SECONDS_PER_DAY for clarity and maintainability.
In `@tests/nav/nav_model/test_nav_model_rings_simulated.py`:
- Around line 69-94: The test helper _make_simulated_model currently uses
NavModelRingsSimulated.__new__ to bypass __init__ and manually set internal
attrs (_config, _obs, _models, _metadata, _ring_name, _sim_params, _logger); add
a short inline comment above that call explaining why __init__ is bypassed
(e.g., "bypass __init__ to avoid complex observation construction in tests") or,
preferably, replace the bypass with a more maintainable approach by using
unittest.mock.patch.object to stub NavModelRingsSimulated.__init__ (or specific
methods) so you can call the real constructor and then patch only the behavior
you need, referencing _make_simulated_model, NavModelRingsSimulated, and the
listed internal attribute names to locate the code to change.
In `@tests/nav/ui/test_common.py`:
- Around line 62-97: Add tests exercising the new layout parameters in
tests/nav/ui/test_common.py by calling build_stretch_controls with
value_label_min_width and slider_horizontal_stretch variants: (1) assert a
TypeError is raised when value_label_min_width is a bool (e.g.,
value_label_min_width=True) with a message like "value_label_min_width must be
int or float, not bool"; (2) assert a ValueError is raised for non-positive
value_label_min_width (e.g., value_label_min_width=0) with a message like
"value_label_min_width must be a positive number"; and (3) assert a ValueError
(or TypeError per implementation) is raised for negative
slider_horizontal_stretch (e.g., slider_horizontal_stretch=-1) with an
appropriate message—place these alongside the existing tests that call
build_stretch_controls so they exercise the same QFormLayout setup and use the
same assertion patterns.
- Around line 54-59: The test uses pytest.approx unnecessarily for integer
slider values; update test_build_stretch_controls_accepts_valid_numeric_args to
assert exact integer equality by comparing result['slider_black'].value() and
result['slider_white'].value() directly to 100 and 900 respectively (locate the
assertions in the test function referencing build_stretch_controls and replace
pytest.approx assertions with direct == comparisons).
---
Duplicate comments:
In `@src/nav/nav_model/rings/ring_math.py`:
- Around line 260-265: The function should reject fade_width_pix == 0 before
reaching the integral cases to avoid compute_fade_integral() being called with
width == 0; in the validation block (around the checks that coerce
fade_width_pix to fwp) add a guard after fwp = float(fade_width_pix) that raises
ValueError (with a clear message) when fwp == 0.0 (or alternatively
short-circuit in compute_edge_fade to treat 0 as “no fade”); reference
compute_edge_fade, compute_fade_integral, and the local variable fwp when
locating where to add the check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2cc130fe-4232-45df-9142-b9fb6c447e08
📒 Files selected for processing (31)
.cursor/rules/documentation.mdc.gitignoredocs/developer_guide.rstdocs/developer_guide_class_hierarchy.rstdocs/developer_guide_configuration.rstdocs/developer_guide_navigation_models.rstdocs/developer_guide_navigation_models_bodies.rstdocs/developer_guide_navigation_models_rings.rstdocs/developer_guide_navigation_models_stars.rstdocs/developer_guide_navigation_models_titan.rstdocs/introduction_configuration.rstsrc/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_model/rings/__init__.pysrc/nav/nav_model/rings/ring_feature.pysrc/nav/nav_model/rings/ring_filter.pysrc/nav/nav_model/rings/ring_math.pysrc/nav/nav_model/rings/ring_render_context.pysrc/nav/nav_model/rings/ring_types.pysrc/nav/ui/common.pysrc/nav/ui/manual_nav_dialog.pytests/nav/nav_model/test_nav_model_rings.pytests/nav/nav_model/test_nav_model_rings_simulated.pytests/nav/nav_model/test_ring_feature.pytests/nav/nav_model/test_ring_filter.pytests/nav/nav_model/test_ring_math.pytests/nav/nav_model/test_ring_render_context.pytests/nav/nav_model/test_ring_types.pytests/nav/ui/__init__.pytests/nav/ui/test_common.py
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/nav/nav_model/nav_model_rings_simulated.py (2)
146-209:⚠️ Potential issue | 🟠 MajorHonor
never_create_modelbefore rendering.This path always calls
render_ring(), optionally builds annotations, and appends aNavModelResult, so metadata-only mode still produces a rendered model in the simulated implementation. Add an early return before the render/annotation block whennever_create_modelis true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/nav_model/nav_model_rings_simulated.py` around lines 146 - 209, The simulated ring path currently always renders and appends a NavModelResult; to honor never_create_model add an early exit before the rendering/annotation block: check the flag (never_create_model) right after computing ring_mask or immediately before the render_ring call and if true skip render_ring, skip _create_simulated_edge_annotations, and do not append NavModelResult (but preserve any metadata-only updates such as self._metadata['confidence'] as needed). Locate the block that calls render_ring, _create_simulated_edge_annotations, and constructs NavModelResult and wrap it in an if not never_create_model: ... (or return early) to prevent creating the model when the flag is set.
149-181:⚠️ Potential issue | 🟠 MajorValidate GUI numeric scalars before
float()conversion.
center_v,center_u, andrangecome from external JSON, butfloat()will accept booleans and non-finite values here, and bad strings surface as unscoped conversion errors. Reject invalid values with clear field-specificValueErrors before using them in geometry or metadata.As per coding guidelines: "Validate inputs at the public API boundary of the library. Raise clear
ValueErrororTypeErrorexceptions for invalid arguments."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/nav_model/nav_model_rings_simulated.py` around lines 149 - 181, The code calls float() directly on external fields center_v, center_u, and range (used in center_v_data/center_u_data and range_val) which will silently accept booleans, non-finite values, or raise unclear conversion errors for bad strings; add explicit validation before conversion: check the presence and type of p['center_v'], p['center_u'], and p.get('range') (reject None/bool/empty strings), ensure numeric types or numeric-string formats, reject NaN/inf with math.isfinite, and raise clear ValueError or TypeError with field-specific messages (e.g., "center_v must be a finite number") before creating center_v_data/center_u_data, building ring_params_extfov, or passing values into render_ring/range_val so downstream geometry and metadata only see validated finite floats.src/nav/nav_model/nav_model_rings.py (1)
276-308:⚠️ Potential issue | 🟠 MajorDo not make ring-config validation depend on FOV visibility.
The
bp_radii.is_all_masked()early return happens beforeRingFeature.from_config()andvalidate_no_date_overlaps(). That means malformed feature blocks and overlapping-date authoring errors are silently skipped whenever the current observation has no visible ring pixels, even though both helpers are meant to fail loudly on bad config. Parse and cross-validatefeaturesfirst, then use visibility only to skip filtering/rendering work.As per coding guidelines: "NEVER trust external input (function arguments from callers, file contents, environment variables, data from remote URLs)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/nav_model/nav_model_rings.py` around lines 276 - 308, The current early-return when bp_radii.is_all_masked() prevents parsing/validation of features and hides config errors; move the loop that builds features (iterate features_dict and call RingFeature.from_config for each key) and the call to validate_no_date_overlaps(features) to occur before the visibility check so malformed feature blocks and date-overlap errors are raised regardless of FOV, then after validation use bp_radii.is_all_masked() to decide whether to log "No rings visible" and either return (honoring always_create_model by appending _create_empty_model_result to _models) or continue with rendering/filtering; ensure you keep the same error messages and behavior of always_create_model/_models.append/_create_empty_model_result.
♻️ Duplicate comments (3)
src/nav/nav_model/rings/ring_render_context.py (2)
91-101:⚠️ Potential issue | 🟠 MajorReject non-2-D
resolutionsarrays here.The docstring and downstream render math treat
resolutionsas a per-pixel 2-D grid, but__post_init__still accepts 1-D/3-D arrays as long as dtype and size pass. That pushes shape failures into later broadcasting/indexing instead of failing at construction.As per coding guidelines: "Validate inputs at the public API boundary of the library. Raise clear
ValueErrororTypeErrorexceptions for invalid arguments."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/nav_model/rings/ring_render_context.py` around lines 91 - 101, The __post_init__ validation for RingsRenderContext currently allows non-2-D arrays; update it to explicitly require a 2-D numpy array by checking self.resolutions.ndim == 2 and raise a TypeError (or ValueError per project convention) if not 2-D, placed after the isinstance check and before dtype/size checks so invalid ranks fail at construction; reference the RingsRenderContext.__post_init__ method and the self.resolutions attribute when making this change.
116-117:⚠️ Potential issue | 🟠 MajorValidate each
all_edge_radiientry, not just the tuple container.Right now any tuple passes here, including malformed items, non-finite radii, empty labels, or unsorted radii.
compute_edge_fade()depends on a sorted sequence of(positive finite radius, non-empty label)pairs, so bad input is still leaking past the public boundary.As per coding guidelines: "NEVER trust external input (function arguments from callers, file contents, environment variables, data from remote URLs)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/nav_model/rings/ring_render_context.py` around lines 116 - 117, The tuple-type check for all_edge_radii is insufficient; update RingsRenderContext (the initializer/property that sets self.all_edge_radii) to validate each entry: ensure each element is a 2-tuple (radius, label), radius is a positive finite number (use math.isfinite) and label is a non-empty string, and that the radii are strictly increasing (sorted ascending) before assigning to self.all_edge_radii; if any check fails raise TypeError or ValueError with a clear message so compute_edge_fade() receives a validated, sorted sequence of (radius, label) pairs.src/nav/nav_model/nav_model_rings.py (1)
229-232:⚠️ Potential issue | 🟠 MajorWrap
epochparsing in planet-scoped validation.
utc_to_et()is still called on whateverplanet_config["epoch"]contains. Non-string values or malformed timestamps will bubble up as downstream parse errors instead of the clear planet/key-specificValueErrorused for the numeric config entries.As per coding guidelines: "Validate inputs at the public API boundary of the library. Raise clear
ValueErrororTypeErrorexceptions for invalid arguments."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nav/nav_model/nav_model_rings.py` around lines 229 - 232, The code currently calls utc_to_et(epoch_str) without validating planet_config['epoch']; wrap the parsing in validation for the epoch value: check that epoch_str is a str (and non-empty), then call utc_to_et inside a try/except that catches parsing errors (ValueError/TypeError) and re-raises a clearer ValueError that includes the planet identifier and the key name (e.g., f"Invalid epoch for planet {planet}: ..."); update the block around epoch_str / utc_to_et so non-string or malformed timestamps produce a planet-scoped ValueError instead of leaking lower-level parse exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nav/nav_model/rings/ring_math.py`:
- Around line 260-264: The validator for fade_width_pix currently allows zero
which later causes compute_fade_integral(width==0) to fail; update the check in
the function handling fade_width_pix (the validation block that converts to fwp)
to require fwp > 0.0 (strictly positive) and raise a ValueError with a clear
message (e.g., "fade_width_pix must be finite and > 0") instead of allowing
zero; this ensures callers of compute_fade_integral receive only valid positive
widths and avoids downstream failures.
- Around line 311-325: The boundary case where fade_end lands exactly on
pixel_upper (shade_above) or pixel_lower (shade_below) is being excluded by
strict > / < checks, so update the final eq_case4 condition to include equality;
in ring_math.py adjust the eq_case4 accumulation: when shade_above use
(edge_radius <= pixel_lower) & (fade_end >= pixel_upper) and when not
shade_above use (edge_radius >= pixel_upper) & (fade_end <= pixel_lower) so
those boundary-aligned full-coverage pixels are classified into eq_case4
(references: shade_above, fade_end, eq2, eq3, eq_case4, pixel_lower,
pixel_upper, edge_radius).
- Around line 174-189: compute_fade_integral currently doesn't validate the
integration bounds a0 and a1; convert a0 and a1 to arrays (e.g., via
np.asarray(..., dtype=np.float64)), check they contain only finite values
(np.isfinite), verify they have identical shapes to each other and to
width/resolutions (raise ValueError if shapes mismatch), and raise a clear
ValueError if any entries are non-finite or shapes differ; keep the validations
consistent with the existing checks for w and res_c and return the result as
np.asarray(..., dtype=np.float64).
In `@src/nav/ui/manual_nav_dialog.py`:
- Around line 48-58: The docstrings for _bilinear_interpolate_fov, the three
_on_image_*_changed() handlers, and the reset handlers are missing the repo's
required Google-style contract; update each to include clear "Parameters:",
"Returns:", and "Raises:" sections and a behavioral note that lists UI side
effects (which labels are updated, which controls are synchronized, and whether
overlays/redraws are triggered). For _bilinear_interpolate_fov explicitly
document arr: NDArrayFloatType, img_v: float, img_u: float, the returned float,
and any ValueError/IndexError raised for out-of-bounds coordinates; for each
_on_image_*_changed handler describe the incoming event/parameter, what internal
state it mutates, which GUI widgets/labels it updates, and that it triggers
overlay redraws; for reset handlers list what state is reset, which
controls/labels are set to defaults, and whether a redraw is invoked. Ensure
wording is precise enough for a black-box test to assert parameter types, return
value, raised exceptions, and all UI side effects.
---
Outside diff comments:
In `@src/nav/nav_model/nav_model_rings_simulated.py`:
- Around line 146-209: The simulated ring path currently always renders and
appends a NavModelResult; to honor never_create_model add an early exit before
the rendering/annotation block: check the flag (never_create_model) right after
computing ring_mask or immediately before the render_ring call and if true skip
render_ring, skip _create_simulated_edge_annotations, and do not append
NavModelResult (but preserve any metadata-only updates such as
self._metadata['confidence'] as needed). Locate the block that calls
render_ring, _create_simulated_edge_annotations, and constructs NavModelResult
and wrap it in an if not never_create_model: ... (or return early) to prevent
creating the model when the flag is set.
- Around line 149-181: The code calls float() directly on external fields
center_v, center_u, and range (used in center_v_data/center_u_data and
range_val) which will silently accept booleans, non-finite values, or raise
unclear conversion errors for bad strings; add explicit validation before
conversion: check the presence and type of p['center_v'], p['center_u'], and
p.get('range') (reject None/bool/empty strings), ensure numeric types or
numeric-string formats, reject NaN/inf with math.isfinite, and raise clear
ValueError or TypeError with field-specific messages (e.g., "center_v must be a
finite number") before creating center_v_data/center_u_data, building
ring_params_extfov, or passing values into render_ring/range_val so downstream
geometry and metadata only see validated finite floats.
In `@src/nav/nav_model/nav_model_rings.py`:
- Around line 276-308: The current early-return when bp_radii.is_all_masked()
prevents parsing/validation of features and hides config errors; move the loop
that builds features (iterate features_dict and call RingFeature.from_config for
each key) and the call to validate_no_date_overlaps(features) to occur before
the visibility check so malformed feature blocks and date-overlap errors are
raised regardless of FOV, then after validation use bp_radii.is_all_masked() to
decide whether to log "No rings visible" and either return (honoring
always_create_model by appending _create_empty_model_result to _models) or
continue with rendering/filtering; ensure you keep the same error messages and
behavior of always_create_model/_models.append/_create_empty_model_result.
---
Duplicate comments:
In `@src/nav/nav_model/nav_model_rings.py`:
- Around line 229-232: The code currently calls utc_to_et(epoch_str) without
validating planet_config['epoch']; wrap the parsing in validation for the epoch
value: check that epoch_str is a str (and non-empty), then call utc_to_et inside
a try/except that catches parsing errors (ValueError/TypeError) and re-raises a
clearer ValueError that includes the planet identifier and the key name (e.g.,
f"Invalid epoch for planet {planet}: ..."); update the block around epoch_str /
utc_to_et so non-string or malformed timestamps produce a planet-scoped
ValueError instead of leaking lower-level parse exceptions.
In `@src/nav/nav_model/rings/ring_render_context.py`:
- Around line 91-101: The __post_init__ validation for RingsRenderContext
currently allows non-2-D arrays; update it to explicitly require a 2-D numpy
array by checking self.resolutions.ndim == 2 and raise a TypeError (or
ValueError per project convention) if not 2-D, placed after the isinstance check
and before dtype/size checks so invalid ranks fail at construction; reference
the RingsRenderContext.__post_init__ method and the self.resolutions attribute
when making this change.
- Around line 116-117: The tuple-type check for all_edge_radii is insufficient;
update RingsRenderContext (the initializer/property that sets
self.all_edge_radii) to validate each entry: ensure each element is a 2-tuple
(radius, label), radius is a positive finite number (use math.isfinite) and
label is a non-empty string, and that the radii are strictly increasing (sorted
ascending) before assigning to self.all_edge_radii; if any check fails raise
TypeError or ValueError with a clear message so compute_edge_fade() receives a
validated, sorted sequence of (radius, label) pairs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 326caf46-3c20-4f31-bf3d-f745a5238af8
📒 Files selected for processing (5)
src/nav/nav_model/nav_model_rings.pysrc/nav/nav_model/nav_model_rings_simulated.pysrc/nav/nav_model/rings/ring_math.pysrc/nav/nav_model/rings/ring_render_context.pysrc/nav/ui/manual_nav_dialog.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactor planetary ring navigation model into
nav.nav_model.ringssubpackagePurpose
The previous
NavModelRingsimplementation mixed orchestration, filtering, orbital/render math, and types in one large module, which was hard to test and extend. This change splits ring logic into a dedicated subpackage with clear boundaries (types, domain/rendering, filtering, pure math), replaces coarse integration tests with focused unit tests, updates Saturn ring YAML to match the stricter config model, and documents the ring configuration and API surface. A configurable log level for ring model creation aligns with other navigation models (e.g. stars).Changes / Implementation Details
nav.nav_model.ringswith:ring_types—RingFeatureType,RingBaseOrbitMode,RingPerturbationMode,RingEdgeDataring_feature—RingFeature,from_config, backplane rendering, date-overlap validationring_filter— four-pass filter (date, radius, resolvability, fade conflict)ring_math—compute_antialiasing,compute_edge_fade,compute_fade_integralring_render_context/ring_render_result— immutable context and render outputsrings/__init__.pyNavModelRings— thin orchestrator: load YAML →RingFeature.from_config→ filter →render(context)→NavModelResultlist; usesgeneral.log_level_model_ringsforPdsLogger.open()andrings_subpackage_log_level()so stdlib loggers undernav.nav_model.ringshonor the same level during creation.NavModelRingsBase— shared annotation helpers andrings_subpackage_log_levelcontext manager.NavModelRingsSimulated— uses shared types/helpers; pixel-spacerender_ringpath preserved.RingPerturbationMode—mode_nummay be zero or negative (required for existingconfig_20_saturn_rings.yaml); still rejectsbooland non-integers;amplituderemains non-negative.general.log_level_model_ringsinconfig_01_settings.yaml(defaultDEBUGin tree).test_nav_model_rings_antialiasing,_edge_fade,_feature_filtering; addconftest.py,test_nav_model_rings.py,test_nav_model_rings_simulated.py,test_ring_*modules for types, feature, filter, math, render context.api_nav_model.rstdocumentsnav.nav_model.rings;developer_guide_configuration.rstanddeveloper_guide_class_hierarchy.rstexpanded;introduction_configuration.rstmentionslog_level_model_rings.build_stretch_controlsinput validation andSTRETCH_CONTROLS_ROW_SPACING;manual_nav_dialogbilinear helper and checkbox state helper..cursorrules/skills updates, dependency lock file, minornavigate_image_filestouch, PDS4 templatedata_orig2.lblx, large refresh ofconfig_20_saturn_rings.yaml.Type of Change
mode_numvalidation vs real YAML)Testing
pytest tests/nav/nav_model/(ring modules and orchestrator; run withpytest -n autoper project convention)OOPS_RESOURCESand dataset fixtures are availablenav_offseton representative images (recommended before release)New/updated tests:
test_ring_types,test_ring_feature,test_ring_filter,test_ring_math,test_ring_render_context,test_nav_model_rings,test_nav_model_rings_simulated, sharedconftest.py.Manual: Run
nav_offset(orcompute_ring_models) against Saturn data after merge; confirm ring models build and logging level behaves as expected whengeneral.log_level_model_ringsis changed.Local verification:
./scripts/run-all-checks.sh(lint, format check, mypy, pytest, Sphinx, markdown lint). The same checks run automatically in CI on the PR.Potential Impacts
nav.nav_model.rings(or symbols re-exported fromnav.nav_modelif applicable) for ring types/helpers instead of relying on private names inside the old monolithicnav_model_rings.py.RingFeature.from_configmay surface authoring errors earlier; Saturn config in-repo is updated to match.RingsRenderContextis built once per observation batch in the orchestrator; fade integral work uses masked arrays where applicable.nav_model_rings.pyinternals will need to retarget the new modules.Checklist
run-all-checks.sh/ CICHANGES.mdor changelog updated if the project requires an entry for this releaserun-all-checks.sh/ CI)Notes
config_20_saturn_rings.yamland the lock file; review the ring Python modules and tests first for logic..github/pull_request_template.mdwas not present in the repo; this file follows.cursor/rules/pull_request.mdcsections.nav_default_config.yamlexample line forlog_level_model_ringsif maintainers want it mirrored besidelog_level_model_starsfor local overrides only.Summary by CodeRabbit
New Features
Documentation
Configuration