ENH: Improve docs navigation and add demo data CLI#56
Merged
Conversation
Floating box was obscuring text. Now disclaimer is integrated with main text on top-level page.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (4)
WalkthroughAdds a new download_data CLI (code, console script, tests), integrates CLI docs, introduces an Isaac for Healthcare docs hub, rewrites BYOD tutorials, switches Sphinx to load version from pyproject.toml and token-replaces version placeholders, and updates developer venv guidance and styling. ChangesDownload Data CLI Feature
Sphinx Versioning and Token Replacement
Documentation Expansion: Isaac for Healthcare Hub
BYOD Tutorials and Content Revision
Developer Infrastructure and Styling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR improves the PhysioMotion4D documentation IA/navigation and adds a new installed CLI entry point for downloading demo datasets, with accompanying docs and tests.
Changes:
- Add
physiomotion4d-download-dataconsole script + CLI module and targeted CLI tests. - Refresh docs navigation (new “Isaac for Healthcare” section), add a homepage version display sourced from
pyproject.toml, and update BYOD tutorial content. - Update CLI/API doc indexes and contributor command guidance to reflect the new entry point and venv workflow.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_download_data_cli.py | Adds focused tests for default and custom download directory behavior. |
| tests/test_cli_smoke.py | Includes the new CLI module in the --help smoke test list. |
| src/physiomotion4d/cli/download_data.py | Implements the new dataset download CLI wrapper. |
| src/physiomotion4d/cli/init.py | Exposes download_data in the CLI package exports. |
| pyproject.toml | Registers physiomotion4d-download-data as an installed console script. |
| docs/isaac_for_healthcare.rst | Adds an “Isaac for Healthcare” landing page for related workflows/assets. |
| docs/index.rst | Updates homepage hero (version display), navigation cards, adds new toctree entries, and moves clinical-use notice. |
| docs/conf.py | Reads project version from pyproject.toml and injects it into docs sources. |
| docs/cli_scripts/overview.rst | Adds the new download command to the CLI scripts overview. |
| docs/cli_scripts/download_data.rst | Documents supported dataset(s), usage, options, and outputs for the download CLI. |
| docs/cli_scripts/byod_tutorials.rst | Refreshes BYOD tutorial instructions and examples to match current CLIs/APIs. |
| docs/api/cli/index.rst | Adds the new CLI module to the API CLI module index. |
| docs/api/cli/download_data.rst | Adds autodoc page for physiomotion4d.cli.download_data. |
| docs/API_MAP.md | Regenerates/updates the API map to include the new CLI and tests. |
| docs/_static/custom.css | Styles the new homepage version line in the hero section. |
| AGENTS.md | Updates contributor guidance to prefer the repo-local .\venv and python -m ... commands. |
| .gitignore | Ignores an additional docs build output directory name. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+27
to
+33
| parser.add_argument( | ||
| "data_name", | ||
| nargs="?", | ||
| choices=[SLICER_HEART_CT], | ||
| default=SLICER_HEART_CT, | ||
| help=f"Dataset to download (default: {SLICER_HEART_CT})", | ||
| ) |
Comment on lines
+197
to
+199
| Not validated for clinical use. PhysioMotion4D 2026.05.07 beta is a research | ||
| and visualization toolkit, not a medical device. Do not use it for diagnosis, | ||
| treatment planning, or clinical decision-making. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
docs/cli_scripts/byod_tutorials.rst (1)
32-38:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix count mismatch in CLI verification sentence.
Line 32 says “both relevant CLI entry-points,” but three commands are listed below.
Proposed wording fix
-Verify that both relevant CLI entry-points are available after installation: +Verify that the relevant CLI entry points are available after installation:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/cli_scripts/byod_tutorials.rst` around lines 32 - 38, Update the verification sentence to match the actual number of commands listed: replace "both relevant CLI entry-points" with wording that covers three commands (e.g., "the following CLI entry-points" or "all three relevant CLI entry-points") so it correctly refers to physiomotion4d-download-data, physiomotion4d-convert-image-to-usd and physiomotion4d-convert-vtk-to-usd.
🧹 Nitpick comments (1)
docs/index.rst (1)
197-199: ⚡ Quick winUse the same version template token in the clinical disclaimer.
This text hardcodes
2026.05.07 beta, which will go stale. Reuse{{ pm4d_project_version }}to keep homepage version references consistent.Proposed change
-Not validated for clinical use. PhysioMotion4D 2026.05.07 beta is a research +Not validated for clinical use. PhysioMotion4D {{ pm4d_project_version }} is a research and visualization toolkit, not a medical device. Do not use it for diagnosis, treatment planning, or clinical decision-making.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/index.rst` around lines 197 - 199, Replace the hardcoded version string in the clinical disclaimer ("PhysioMotion4D 2026.05.07 beta") with the project template token {{ pm4d_project_version }} so the homepage disclaimer uses the same dynamic version; locate the disclaimer text block that starts "Not validated for clinical use..." (the clinical disclaimer paragraph) and substitute the literal version with the token, preserving surrounding wording and punctuation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/cli_scripts/byod_tutorials.rst`:
- Around line 302-309: The table rows for assets like "Chest with cardiac
motion", "Chest with respiratory motion", "Heart with cardiac motion", and
"Lungs with respiratory motion" have empty "Download link" cells; update the
table in docs/cli_scripts/byod_tutorials.rst so each asset either has a valid
URL or an explicit placeholder (e.g., "TBD", "Link coming soon", or "N/A") in
the "Download link" column, ensuring the placeholder text is consistent across
all four entries and clearly marks the link as unavailable until real URLs are
provided.
In `@docs/cli_scripts/download_data.rst`:
- Around line 54-59: Update the documentation line that currently lists
"data/Slicer-Heart-CT/TruncalValve_4DCT.seq.nrrd" to clarify the output respects
the --directory option by showing it as "<directory>/TruncalValve_4DCT.seq.nrrd"
(and optionally mention the default "data/" directory); reference the example
dataset name "Slicer-Heart-CT" and the file "TruncalValve_4DCT.seq.nrrd" so
readers know the file name but see that the parent path comes from the
--directory flag.
In `@docs/conf.py`:
- Around line 235-236: Annotate the new Sphinx hook(s) with explicit
mypy-compatible type hints: change the signature of
_replace_project_version_token to accept a Sphinx app object, a docname string,
and the source list and return None (e.g. _app: Sphinx, _docname: str, source:
list[str]) -> None, and do the same for the other new hook functions referenced
around lines 239-241; also add the minimal imports/TYPE_CHECKING guard needed
(e.g. from sphinx.application import Sphinx or a forward-reference) so the
annotations pass strict mypy (disallow_untyped_defs) checks.
In `@src/physiomotion4d/cli/download_data.py`:
- Line 45: Replace the print call used for CLI status reporting with the
project's logger: locate the print(f"Downloaded {SLICER_HEART_CT} to:
{data_file}") in download_data.py (within the download routine/function) and
change it to use the module logger (e.g., logger.info or logger.debug as
appropriate) so the message includes SLICER_HEART_CT and data_file; if a logger
is not already defined in this module, add a standard
logging.getLogger(__name__) instance at module scope and use it for the status
message.
---
Outside diff comments:
In `@docs/cli_scripts/byod_tutorials.rst`:
- Around line 32-38: Update the verification sentence to match the actual number
of commands listed: replace "both relevant CLI entry-points" with wording that
covers three commands (e.g., "the following CLI entry-points" or "all three
relevant CLI entry-points") so it correctly refers to
physiomotion4d-download-data, physiomotion4d-convert-image-to-usd and
physiomotion4d-convert-vtk-to-usd.
---
Nitpick comments:
In `@docs/index.rst`:
- Around line 197-199: Replace the hardcoded version string in the clinical
disclaimer ("PhysioMotion4D 2026.05.07 beta") with the project template token {{
pm4d_project_version }} so the homepage disclaimer uses the same dynamic
version; locate the disclaimer text block that starts "Not validated for
clinical use..." (the clinical disclaimer paragraph) and substitute the literal
version with the token, preserving surrounding wording and punctuation.
🪄 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: CHILL
Plan: Pro
Run ID: 96e641f7-bdef-4fec-b0dd-68bb5bdfb14e
📒 Files selected for processing (17)
.gitignoreAGENTS.mddocs/API_MAP.mddocs/_static/custom.cssdocs/api/cli/download_data.rstdocs/api/cli/index.rstdocs/cli_scripts/byod_tutorials.rstdocs/cli_scripts/download_data.rstdocs/cli_scripts/overview.rstdocs/conf.pydocs/index.rstdocs/isaac_for_healthcare.rstpyproject.tomlsrc/physiomotion4d/cli/__init__.pysrc/physiomotion4d/cli/download_data.pytests/test_cli_smoke.pytests/test_download_data_cli.py
Comment on lines
+302
to
+309
| * - Chest with cardiac motion | ||
| - | ||
| * - Chest with respiratory motion | ||
| - | ||
| * - Heart with cardiac motion | ||
| - | ||
| * - Lungs with respiratory motion | ||
| - |
There was a problem hiding this comment.
Populate or explicitly mark missing asset download links.
The “Download link” column is blank for all assets, which leaves the section non-actionable.
Proposed minimal fix (until links are ready)
* - Chest with cardiac motion
- -
+ - Coming soon
* - Chest with respiratory motion
- -
+ - Coming soon
* - Heart with cardiac motion
- -
+ - Coming soon
* - Lungs with respiratory motion
- -
+ - Coming soon📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| * - Chest with cardiac motion | |
| - | |
| * - Chest with respiratory motion | |
| - | |
| * - Heart with cardiac motion | |
| - | |
| * - Lungs with respiratory motion | |
| - | |
| * - Chest with cardiac motion | |
| - Coming soon | |
| * - Chest with respiratory motion | |
| - Coming soon | |
| * - Heart with cardiac motion | |
| - Coming soon | |
| * - Lungs with respiratory motion | |
| - Coming soon |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/cli_scripts/byod_tutorials.rst` around lines 302 - 309, The table rows
for assets like "Chest with cardiac motion", "Chest with respiratory motion",
"Heart with cardiac motion", and "Lungs with respiratory motion" have empty
"Download link" cells; update the table in docs/cli_scripts/byod_tutorials.rst
so each asset either has a valid URL or an explicit placeholder (e.g., "TBD",
"Link coming soon", or "N/A") in the "Download link" column, ensuring the
placeholder text is consistent across all four entries and clearly marks the
link as unavailable until real URLs are provided.
Comment on lines
+54
to
+59
| For ``Slicer-Heart-CT``, the command downloads or reuses: | ||
|
|
||
| .. code-block:: text | ||
|
|
||
| data/Slicer-Heart-CT/TruncalValve_4DCT.seq.nrrd | ||
|
|
There was a problem hiding this comment.
Clarify output path to respect --directory.
Line 54 currently implies a fixed output location, but Line 47-Line 49 documents a configurable destination. Please describe the output as <directory>/TruncalValve_4DCT.seq.nrrd (and optionally note the default directory).
Suggested doc tweak
-For ``Slicer-Heart-CT``, the command downloads or reuses:
+For ``Slicer-Heart-CT``, the command downloads or reuses:
.. code-block:: text
- data/Slicer-Heart-CT/TruncalValve_4DCT.seq.nrrd
+ <directory>/TruncalValve_4DCT.seq.nrrd
+
+By default, ``<directory>`` is ``data/Slicer-Heart-CT``.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| For ``Slicer-Heart-CT``, the command downloads or reuses: | |
| .. code-block:: text | |
| data/Slicer-Heart-CT/TruncalValve_4DCT.seq.nrrd | |
| For ``Slicer-Heart-CT``, the command downloads or reuses: | |
| .. code-block:: text | |
| <directory>/TruncalValve_4DCT.seq.nrrd | |
| By default, ``<directory>`` is ``data/Slicer-Heart-CT``. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/cli_scripts/download_data.rst` around lines 54 - 59, Update the
documentation line that currently lists
"data/Slicer-Heart-CT/TruncalValve_4DCT.seq.nrrd" to clarify the output respects
the --directory option by showing it as "<directory>/TruncalValve_4DCT.seq.nrrd"
(and optionally mention the default "data/" directory); reference the example
dataset name "Slicer-Heart-CT" and the file "TruncalValve_4DCT.seq.nrrd" so
readers know the file name but see that the parent path comes from the
--directory flag.
Comment on lines
+235
to
+236
| def _replace_project_version_token(_app, _docname, source): | ||
| source[0] = source[0].replace("{{ pm4d_project_version }}", release) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add explicit type hints to the new Sphinx hook function signatures.
The new callable(s) are untyped; please annotate them to align with strict typing expectations.
Proposed change
-def _replace_project_version_token(_app, _docname, source):
+def _replace_project_version_token(
+ _app: object, _docname: str, source: list[str]
+) -> None:
source[0] = source[0].replace("{{ pm4d_project_version }}", release)
@@
-def setup(app):
+def setup(app: object) -> None:
"""Custom setup function for Sphinx."""Also applies to: 239-241
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/conf.py` around lines 235 - 236, Annotate the new Sphinx hook(s) with
explicit mypy-compatible type hints: change the signature of
_replace_project_version_token to accept a Sphinx app object, a docname string,
and the source list and return None (e.g. _app: Sphinx, _docname: str, source:
list[str]) -> None, and do the same for the other new hook functions referenced
around lines 239-241; also add the minimal imports/TYPE_CHECKING guard needed
(e.g. from sphinx.application import Sphinx or a forward-reference) so the
annotations pass strict mypy (disallow_untyped_defs) checks.
|
|
||
| if args.data_name == SLICER_HEART_CT: | ||
| data_file = DataDownloadTools.DownloadSlicerHeartCTData(output_dir) | ||
| print(f"Downloaded {SLICER_HEART_CT} to: {data_file}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Replace print with logger output for CLI status reporting.
Line 45 uses print(...), which diverges from the project’s Python logging convention.
Proposed change
+import logging
import sys
@@
+LOGGER = logging.getLogger(__name__)
@@
- print(f"Downloaded {SLICER_HEART_CT} to: {data_file}")
+ LOGGER.info('Downloaded %s to: %s', SLICER_HEART_CT, data_file)
return 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| print(f"Downloaded {SLICER_HEART_CT} to: {data_file}") | |
| import logging | |
| import sys | |
| LOGGER = logging.getLogger(__name__) | |
| # ... (intervening code) | |
| LOGGER.info('Downloaded %s to: %s', SLICER_HEART_CT, data_file) | |
| return 0 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/physiomotion4d/cli/download_data.py` at line 45, Replace the print call
used for CLI status reporting with the project's logger: locate the
print(f"Downloaded {SLICER_HEART_CT} to: {data_file}") in download_data.py
(within the download routine/function) and change it to use the module logger
(e.g., logger.info or logger.debug as appropriate) so the message includes
SLICER_HEART_CT and data_file; if a logger is not already defined in this
module, add a standard logging.getLogger(__name__) instance at module scope and
use it for the status message.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
==========================================
+ Coverage 30.61% 30.79% +0.18%
==========================================
Files 49 50 +1
Lines 6808 6826 +18
==========================================
+ Hits 2084 2102 +18
Misses 4724 4724
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
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.
Move the clinical-use disclaimer out of the homepage hero and add project
version display from pyproject metadata. Add Isaac for Healthcare navigation and
refresh the BYOD tutorial with current image, DICOM, VTK, and demo-data usage.
Expose physiomotion4d-download-data as an installed CLI, document its supported
Slicer-Heart-CT dataset, and add focused CLI tests. Update API docs, CLI indexes,
and contributor command guidance to match the new entry point and venv workflow.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores