Doc updates #62
Doc updates #62
Conversation
WalkthroughDocumentation and a few small imports were updated to reflect new CLI entrypoint names, reorganized backplanes module paths, and widespread constructor/method signature renames; developer guides document added ObsSnapshot helpers for bounding-box checks and clipping. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/experiments/backplanes/smoke_backplanes_sim.py (1)
⏰ 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)
🔇 Additional comments (1)
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #62 +/- ##
=======================================
Coverage 36.98% 36.98%
=======================================
Files 57 57
Lines 4350 4350
Branches 564 564
=======================================
Hits 1609 1609
Misses 2641 2641
Partials 100 100 ☔ 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: 2
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
README.md(1 hunks)docs/developer_guide_backplanes.rst(1 hunks)docs/developer_guide_class_hierarchy.rst(5 hunks)docs/developer_guide_extending.rst(4 hunks)docs/user_guide_backplanes.rst(5 hunks)docs/user_guide_navigation.rst(4 hunks)src/experiments/offset_sensitivity/README.md(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-24T02:08:32.443Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 48
File: nav/config_files/config_90_backplanes.yaml:29-47
Timestamp: 2025-11-24T02:08:32.443Z
Learning: In nav/config_files/config_90_backplanes.yaml, the `phase_angle` method is intentionally shared between body and ring backplanes. While other ring backplane methods use ring-specific names (e.g., `ring_emission_angle`, `ring_angular_resolution`), `phase_angle` works for both bodies and rings without needing a `ring_phase_angle` variant.
Applied to files:
docs/developer_guide_backplanes.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-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/developer_guide_class_hierarchy.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:
docs/developer_guide_class_hierarchy.rst
📚 Learning: 2025-09-22T21:03:10.431Z
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 1
File: tests/nav/dataset/test_dataset_pds3_newhorizons_lorri.py:27-33
Timestamp: 2025-09-22T21:03:10.431Z
Learning: PDS3 (Planetary Data System version 3) archives are immutable and never updated once finalized. Exact count assertions in tests for PDS3 datasets are appropriate since the underlying data will never change.
Applied to files:
docs/developer_guide_class_hierarchy.rst
⏰ 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.12)
- GitHub Check: Test nav (ubuntu-latest, 3.11)
- GitHub Check: Test nav (ubuntu-latest, 3.10)
🔇 Additional comments (18)
README.md (1)
82-97: Quick Start examples correctly updated to CLI-first pattern.The examples clearly show installation assumptions via CLI scripts (nav_offset, nav_offset_cloud_tasks) with appropriate environment-configuration flags. Arguments are consistent with documented CLI interface.
docs/user_guide_navigation.rst (3)
169-169: CLI command examples are consistent across the guide.All example commands correctly use the CLI script names (
nav_offset,nav_offset_cloud_tasks) with appropriate arguments. Dataset selection patterns (positional image names,--volumes,--first-volume,--last-volume, etc.) and output options are documented consistently.Also applies to: 175-175, 178-178, 189-189, 245-245, 251-251, 257-257, 263-263, 269-269
274-274: Cloud-tasks entry point and invocation are correctly documented.The cloud-tasks variant (
nav_offset_cloud_tasks) is properly described at line 274, and the invocation syntax at line 278 correctly omits dataset selection flags (as they come from task payloads).Also applies to: 278-278
8-8: CLI script documentation is clear and comprehensive.Lines 8 and 185 appropriately document the shift from Python module invocation to installed CLI scripts. The introduction explains that
nav_offsetis exposed viapyproject.toml, setting clear expectations for users installing the package. Entry pointsnav_offsetandnav_offset_cloud_tasksare properly registered in the project configuration.docs/developer_guide_class_hierarchy.rst (6)
14-18: NavBase keyword-only arguments correctly documented.Line 15 documents
NavBase.__init__signature as(*, config=None, **kwargs), establishing the pattern for all subclasses. This is consistent with the modernized API described across extending and backplanes guides.
72-76: Obs and ObsSnapshot hierarchy updates reflect new methods and organization.Lines 72–76 document new
ObsSnapshotmethods (inventory_body_in_fov, etc.), and lines 81–92 showObsInstas an abstract base withfrom_file(path, *, config=None, extfov_margin_vu=None, **kwargs)and additional metadata methods. The diagram correctly reflects these additions. Verify the actual class implementations match these signatures.Also applies to: 81-92
115-115: Keyword-only argument pattern is consistently applied across all major classes.Lines 115 (NavMaster), 125 (NavModel), 171 (NavTechnique), and 198 (Annotation) all show updated constructors with keyword-only arguments and config=None defaults. This modernization is consistent across the entire hierarchy and represents a cohesive API refactor.
Also applies to: 125-125, 171-171, 198-198
215-219: Config class methods correctly documented.Lines 215–219 update the
Configclass signature to__init__()with new methods:read_config(config_path=None, reread=False),update_config(config_path, read_default=True), andcategory(name). This represents a shift from a legacy load/get/save pattern to a more explicit configuration management API. Verify these methods exist and are correctly implemented insrc/nav/support/config.py.#!/bin/bash # Verify Config class methods grep -E "def read_config|def update_config|def category" src/nav/support/config.py
267-336: Key Components section provides updated narrative and class references.Lines 267–336 update the narrative descriptions of major components (NavBase, NavMaster, NavModel, NavTechnique, Dataset, Obs/ObsSnapshot, Annotation) with references to fully qualified class names and updated API descriptions. Cross-reference these descriptions with the actual class implementations to ensure accuracy.
#!/bin/bash # Spot-check that referenced classes exist for class_ref in "nav.support.nav_base.NavBase" "nav.nav_master.nav_master.NavMaster" "nav.nav_model.nav_model.NavModel"; do echo "Checking: $class_ref" module=$(echo $class_ref | sed 's/\./\//g' | sed 's/$/.py/') [ -f "src/$module" ] && echo " ✓ File exists" || echo " ✗ File NOT found" done
230-243: Inheritance relationships in diagram are accurate. All class hierarchy relationships in lines 230–243 correctly reflect the actual class implementations in the codebase: DataSet, DataSetPDS3, DataSetPDS4, and their subclasses properly extend their parent classes; Obs, ObsSnapshot, ObsInst, and ObsSnapshotInst relationships are correctly represented, including ObsSnapshotInst's multiple inheritance from both ObsSnapshot and ObsInst; and all instrument-specific and simulation classes (Cassini ISS, Voyager ISS, Galileo SSI, New Horizons LORRI, and Sim) properly extend their parent classes.src/experiments/offset_sensitivity/README.md (1)
36-36: CLI script references are properly registered and documented.Lines 36 and 87 correctly reference
nav_offset_cloud_tasksas an installed CLI script. The entry point is registered inpyproject.tomlasnav_offset_cloud_tasks = "main.nav_offset_cloud_tasks:main", confirming that the README documentation is accurate and the task output format is correctly specified as compatible with this CLI tool.docs/developer_guide_backplanes.rst (1)
47-52: New ObsSnapshot methods are clearly documented and correctly implemented.All four methods mentioned in lines 47–52 exist in the implementation with matching signatures:
inventory_body_in_fov(inv: dict[str, Any]) -> boolinventory_body_in_extfov(inv: dict[str, Any]) -> boolclip_rect_fov(u_min: int, u_max: int, v_min: int, v_max: int) -> tuple[int, int, int, int]clip_rect_extfov(u_min: int, u_max: int, v_min: int, v_max: int) -> tuple[int, int, int, int]Documentation accurately reflects the implementation.
docs/developer_guide_extending.rst (4)
42-42: Verify PathLike type is available in nav.support.types.Line 42 imports
PathLikefromnav.support.types. Confirm this type is exported and used consistently across the codebase for file path parameters.
74-75: Constructors correctly updated with logger_name removal confirmed.Both
NavModel(lines 74–75) andNavTechnique(lines 97–98) constructors use the keyword-only pattern(*, config=None)withlogger_nameparameter removed. The implementations delegate toNavBase.__init__which provides logging via theloggerproperty and a_loggerinstance variable, eliminating the need for explicit logger_name handling.
20-21: Constructor signatures follow keyword-only pattern; documentation verified against implementations.Lines 20–21 correctly show
DataSetNewInstrument.__init__using keyword-only arguments(*, config=None)with explicitsuper().__init__(config=config)call. Line 24 shows the private static method_img_name_valid(name: str) -> bool:implementation. Verification confirms: DataSetPDS3 uses keyword-only arguments throughout its hierarchy; all dataset classes consistently implement_img_name_validas a private method with type hints;PathLikeis properly defined innav.support.typesasstr | Path | FCPath. Documentation patterns align with implementations across all dataset classes.
45-46: Documentation example at lines 45–46 doesn't match the base class design.The documented
__init__signature showingconfigas an explicit keyword-only parameter(self, obs, *, config=None, **kwargs)does not align withObsSnapshotInst's actual implementation. The base class defines__init__(self, snapshot: Snapshot, **kwargs: Any)without exposingconfigas an explicit parameter. While configuration parameters flow through**kwargsto the parentObsSnapshotclass due to Python's MRO, the documented pattern incorrectly suggestsconfigshould be an explicit parameter. Update the documentation to either: (1) show the pattern without explicit config, passing it through kwargs, or (2) explain that config flows implicitly through kwargs to the parent class.Likely an incorrect or invalid review comment.
docs/user_guide_backplanes.rst (2)
22-23: CLI script references are consistent and properly registered.Lines 22–23, 38, 47–49, and 81 correctly reference installed CLI scripts (
nav_backplanes,nav_backplanes_cloud_tasks,nav_backplane_viewer) without Python module paths. All three entry points are registered inpyproject.tomlunder[project.scripts], and documentation formatting is consistent with similar updates in other user guides.
69-69: The module path reorganization fromsrc/nav/backplanes/tosrc/backplanes/is complete and correctly implemented. The template file exists atsrc/backplanes/templates/backplanes.lblx, and the template loading code insrc/backplanes/writer.pyhas been properly updated to use a relative path:Path(__file__).resolve().parent / 'templates' / 'backplanes.lblx'. This approach ensures the template loads correctly from the new module location. No broken references to old paths exist in the source code.
| class DataSetPDS4 { | ||
| +__init__(*, config=None) | ||
| +_img_name_valid(name) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if DataSetPDS4 exists in src/nav/dataset/
find src/nav/dataset/ -name "*pds4*" -type f 2>/dev/null
# Check if the class is defined
grep -r "class DataSetPDS4" src/ 2>/dev/null
# Check the dataset registry
grep "DataSetPDS4" src/nav/dataset/__init__.py 2>/dev/nullRepository: SETI/rms-nav
Length of output: 152
🏁 Script executed:
#!/bin/bash
# Read the DataSetPDS4 class to verify methods
echo "=== DataSetPDS4 class implementation ==="
head -100 src/nav/dataset/dataset_pds4.py
echo -e "\n=== Check for __init__ and _img_name_valid methods ==="
grep -n "def __init__\|def _img_name_valid" src/nav/dataset/dataset_pds4.py
echo -e "\n=== Check dataset registry in __init__.py ==="
cat src/nav/dataset/__init__.pyRepository: SETI/rms-nav
Length of output: 3799
DataSetPDS4 class exists but is not registered in the dataset registry.
The DataSetPDS4 class is defined in src/nav/dataset/dataset_pds4.py with the _img_name_valid(name) method, but it is missing from the dataset registry. The class is not imported in src/nav/dataset/__init__.py, not registered in _DATASET_NAME_TO_CLASS_MAPPING, and not included in __all__. Additionally, there is no explicit __init__(*, config=None) method in the class definition.
🤖 Prompt for AI Agents
In docs/developer_guide_class_hierarchy.rst around lines 61 to 64: the
DataSetPDS4 class is declared but the implementation
src/nav/dataset/dataset_pds4.py is not registered; open
src/nav/dataset/__init__.py and (1) import DataSetPDS4 from .dataset_pds4, (2)
add an entry mapping its dataset name key to DataSetPDS4 in the
_DATASET_NAME_TO_CLASS_MAPPING, (3) include "DataSetPDS4" in the module __all__
list, and (4) update src/nav/dataset/dataset_pds4.py to define an explicit
__init__(self, *, config=None) initializer (calling super().__init__ if needed)
and ensure the _img_name_valid(self, name) method signature is instance-based;
save and run tests.
There was a problem hiding this comment.
This is an abstract base class. There are no implemented datasets yet.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if DataSetPDS4 is marked as abstract
echo "=== DataSetPDS4 class definition ==="
grep -A 30 "class DataSetPDS4" src/nav/dataset/dataset_pds4.py
echo -e "\n=== Check for ABC or abstractmethod usage ==="
head -20 src/nav/dataset/dataset_pds4.py | grep -E "import.*ABC|import.*abstractmethod|from.*abc"
Length of output: 1225
@rfrenchseti Thank you for the clarification! I can confirm that DataSetPDS4 is correctly implemented as an abstract base class (it inherits from ABC and all methods raise NotImplementedError), so it makes perfect sense that it's not registered in the dataset registry.
For consistency with how other abstract classes are documented in the diagram, you might consider marking DataSetPDS4 with the <<abstract>> stereotype in the Mermaid diagram, similar to how DataSet, Obs, NavModel, etc. are marked. This would make it immediately clear to readers that it's not yet implemented:
class DataSetPDS4 {
<<abstract>>
+__init__(*, config=None)
+_img_name_valid(name)
}However, this is just a suggestion for documentation clarity—the code itself is fine as-is.
🧠 Learnings used
Learnt from: rfrenchseti
Repo: SETI/rms-nav PR: 1
File: tests/nav/dataset/test_dataset_pds3_newhorizons_lorri.py:27-33
Timestamp: 2025-09-22T21:03:10.431Z
Learning: PDS3 (Planetary Data System version 3) archives are immutable and never updated once finalized. Exact count assertions in tests for PDS3 datasets are appropriate since the underlying data will never change.
Minor updates to documentation to adjust to new directory layout and other minor changes.
Summary by CodeRabbit
Documentation
New Features
✏️ Tip: You can customize this high-level summary in your review settings.