Improve documentation with pedagogical approach #625
Conversation
- Add conceptual introductions to four core components (Grid, Medium, Source, Sensor) - Create comprehensive "Your First Simulation" tutorial - Add pedagogical examples guide organized by concept complexity - Reorganize main documentation structure with clear learning progression - Explain example naming conventions (IVP, US, AT, PR, SD, NA) - Maintain focus on four-component framework throughout
|
Looks great—nice work! |
this link should pass the link checker
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #625 +/- ##
=======================================
Coverage 73.95% 73.95%
=======================================
Files 50 50
Lines 7000 7000
Branches 1338 1338
=======================================
Hits 5177 5177
Misses 1280 1280
Partials 543 543
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:
|
|
also: fails as the artefact gets deleted after 90 days, so the link no longer works. |
|
@CodeRabbit-ai review |
|
@waltsims I'll review the changes in this PR focusing on the documentation improvements and pedagogical approach you've outlined. ✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded@waltsims has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 17 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds a GitHub Actions link-check workflow, updates Sphinx config to detect the current branch and add extlinks, reorganizes and expands documentation (many Getting Started pages, examples guide, and API doc headers), updates numerous example/README Colab links, tightens data_cast validation in two angular spectrum utilities, and a related test doc comment. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub
participant GHA as GitHub Actions
participant LC as Lychee Action
Dev->>GH: Push / Open PR
GH-->>GHA: Trigger link-check workflow
GHA->>GHA: actions/checkout@v4 (fetch-depth: 1)
GHA->>LC: Run lycheeverse/lychee-action@v2 with args
Note right of LC #d6f5d6: args include --verbose --no-progress\n--max-concurrency 12 --max-retries 3\n--accept 200,206,403,429\npaths: **/*.md **/*.rst\nexclude: localhost, img.shields.io
LC-->>GHA: Report results
alt Broken links
GHA-->>GH: Set status = failure
else All links valid
GHA-->>GH: Set status = success
end
sequenceDiagram
autonumber
participant Sphinx as Sphinx
participant Conf as docs/conf.py
participant Env as Environment
participant Git as Git CLI
Sphinx->>Conf: Load configuration
Conf->>Env: Read CI/docs env vars (READTHEDOCS_*, GITHUB_REF_NAME, etc.)
alt Branch env found
Conf-->>Conf: Use detected branch
else Branch env not found
Conf->>Git: git rev-parse --abbrev-ref HEAD
alt Git returns branch
Git-->>Conf: branch name
else Git fails
Conf-->>Conf: default to "master"
end
end
Conf-->>Sphinx: Provide extlinks (ghfile/ghdir) using branch
Sphinx->>Sphinx: Resolve :ghfile:`...` → GitHub blob URLs in docs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ Finishing touches
🧪 Generate unit tests
Comment |
Docstrings generation was requested by @waltsims. * #625 (comment) The following files were modified: * `docs/conf.py`
|
Note Generated docstrings for this pull request at #638 |
Docstrings generation was requested by @waltsims. * #625 (comment) The following files were modified: * `docs/conf.py` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/README.md (1)
22-22: Fix broken relative examples link on ReadTheDocsdocs/README.md (line 22) uses a relative link (../examples/) which will 404 on ReadTheDocs — point it at the repository examples directory or an RTD page instead.
-A large [collection of examples](../examples/) exists to get started with k-wave-python. +A large [collection of examples](https://github.com/waltsims/k-wave-python/tree/master/examples) exists to get started with k-wave-python.docs/conf.py (1)
17-27: Add required extensions: sphinx-design (used) and MyST for Markdown.
- The docs use “.. grid::”/“.. grid-item-card::” → needs “sphinx_design”.
- You listed “.md” in source_suffix; without a Markdown parser, builds will warn/fail. Add “myst_parser”.
extensions = [ "sphinx.ext.autodoc", "sphinx.ext.githubpages", "sphinx_toolbox.code", "sphinx_copybutton", "sphinx.ext.coverage", "sphinx.ext.napoleon", "sphinx.ext.viewcode", "sphinx_mdinclude", + "myst_parser", + "sphinx_design", "sphinx.ext.extlinks", # Enables :ghfile: external links ] + +# MyST (Markdown) config +source_suffix = {".rst": "restructuredtext", ".md": "markdown"} +myst_enable_extensions = ["colon_fence", "attrs_block"]
🧹 Nitpick comments (27)
docs/kwave.data.rst (1)
1-3: Preserve inbound refs by adding a stable label for the old heading text.Renaming the top heading can break existing :ref: links if any referenced “kwave.data module”. Add an explicit anchor to keep backward compatibility.
+.. _kwave-data-module: data =================Also run a quick grep to update any hard-coded :ref:
kwave.data moduleusages elsewhere.docs/kwave.utils.rst (1)
1-3: Add an anchor to avoid breaking existing section refs.The title change may invalidate :ref: targets created from the old heading. Add a stable label.
+.. _kwave-utils-package: utils ===================docs/kwave.executor.rst (1)
1-3: Keep a backward-compatible label for refs to the old heading.+.. _kwave-executor-module: executor =====================docs/kwave.enums.rst (1)
1-3: Retain old heading ref via explicit label.+.. _kwave-enums-module: enums ==================docs/kwave.options.rst (1)
1-3: Add compatibility anchor for previous heading text.+.. _kwave-options-module: options ====================docs/kwave.kWaveSimulation_helper.rst (1)
1-3: Section rename: add a label to preserve old refs (“kwave.kWaveSimulation_helper package”).+.. _kwave-kWaveSimulation_helper-package: kWaveSimulation_helper =====================================docs/README.md (2)
3-3: Make the Support badge clickable.Wrap the badge image in a link to the Discord invite so users can reach the server in one click.
- +[](https://discord.gg/<your-invite-code>)Please replace with the real invite.
1-16: Unify brand casing (“k-Wave-python” vs “k-wave-python”).The header and body use mixed casing. Pick one convention and apply consistently across docs/README.
docs/kwave.kWaveSimulation.rst (1)
1-3: Add a compatibility label for the old heading.+.. _kwave-kWaveSimulation-module: kWaveSimulation ============================docs/kwave.kspaceFirstOrderAS.rst (1)
1-2: Nit: Match RST underline length to titleFor consistency with other pages and some linters, align the underline length to the title text length.
-kspaceFirstOrderAS -=============================== +kspaceFirstOrderAS +==================docs/kwave.recorder.rst (1)
1-2: Nit: Adjust underline to match title lengthKeeps style consistent with other pages.
-recorder -===================== +recorder +========docs/kwave.kspaceFirstOrder3D.rst (1)
1-2: Nit: match underline length to title.For consistency with other pages, make the underline length equal to “kspaceFirstOrder3D”.
Apply:
-kspaceFirstOrder3D -=============================== +kspaceFirstOrder3D +==================docs/kwave.ktransducer.rst (1)
1-2: Nit: underline off by one.Match underline length to “kTransducer”.
Apply:
-kTransducer -============ +kTransducer +===========.github/workflows/link-check.yml (3)
31-32: Trim trailing space to satisfy YAML linters.There’s a trailing space at the end of Line 32.
Apply:
- LY_CHEE_EXCLUDE: "http://localhost,https://localhost,https://127.0.0.1,https://img.shields.io" + LY_CHEE_EXCLUDE: "http://localhost,https://localhost,https://127.0.0.1,https://img.shields.io"
21-29: Reduce flakiness on external sites.Add retry flags (above) and consider accepting common non-200 statuses from CDNs.
Optionally append:
--retry-wait-time 4 + --accept 200,206,403,429
3-7: Optional: scope workflow to docs-only changes to save CI minutes.If desired, restrict to docs paths.
For pull_request:
on: pull_request: + paths: + - 'docs/**' + - '.github/workflows/link-check.yml' push: branches: [ main, master ] + paths: + - 'docs/**' + - '.github/workflows/link-check.yml'docs/get_started/source_overview.rst (3)
13-13: Unify variable names for clarity.Use the same name for the initial pressure example.
Apply:
- source.p0 = initial_pressure_map # Same size as grid + source.p0 = initial_pressure_distribution # Same size as gridAlso applies to: 40-41
24-25: Expand stress component list (optional).Consider listing shear terms (sxy, sxz, syz) for completeness in elastic simulations.
19-21: Keep naming consistent for masks.Use transducer_mask consistently in both examples.
Apply:
- source.p_mask = transducer_positions # Binary mask + source.p_mask = transducer_mask # Binary maskAlso applies to: 44-46
docs/examples_guide.rst (1)
133-141: Consistency: “Focused” vs “Focussed”.Titles use “Focused …” while paths include “sd_focussed_detector_2D”. If the folder is intentionally spelled “focussed”, keep the path but standardize the prose to “Focused” across this page.
docs/get_started/sensor_overview.rst (1)
15-21: Clarify Cartesian points semantics and use boolean masks in examples.
- Explicitly state units for point coordinates (meters) and the grid coordinate frame (consistent with grid.x_vec / grid.y_vec).
- Use boolean masks in examples to match typical k-Wave expectations and avoid dtype surprises.
- sensor_points = np.array([[x1, y1], [x2, y2], ...]) # [N_sensors × N_dims] + sensor_points = np.array([[x1, y1], [x2, y2], ...]) # meters; shape [N_sensors × N_dims] in the grid coordinate system - sensor_mask = np.zeros(grid.N) - sensor_mask[64, :] = 1 # Horizontal line + sensor_mask = np.zeros(grid.N, dtype=bool) + sensor_mask[64, :] = True # Horizontal lineAlso applies to: 53-63
docs/get_started/first_simulation.rst (1)
166-174: Avoid relying on grid.Nt/grid.dt unless guaranteed.If kspaceFirstOrder2D doesn’t populate grid.Nt/dt, plotting will break. Prefer time from the returned data if available.
-plt.imshow(sensor_data['p'], aspect='auto', extent=[ - 0, grid.Nt * grid.dt * 1e6, # Time in μs - 0, sensor_data['p'].shape[0] -]) +time_us = (sensor_data.get('t') * 1e6) if 't' in sensor_data else np.arange(sensor_data['p'].shape[1]) # fallback: sample index +plt.imshow(sensor_data['p'], aspect='auto', extent=[0, time_us[-1] if np.ndim(time_us) else time_us, 0, sensor_data['p'].shape[0]])docs/get_started/grid_overview.rst (2)
13-14: Clarify CFL-based time-step formula.Briefly show the relation (e.g., dt = CFL · min(dx)/c_max) and link to the API page that controls CFL.
-**Time Step**: Automatically calculated from CFL stability condition. Can be overridden for custom temporal sampling. +**Time Step**: Computed from the CFL condition (e.g., :math:`\Delta t = \mathrm{CFL}\cdot \min(\Delta x)/c_\mathrm{max}`). You can override CFL for custom temporal sampling. See :doc:`../kwave.options`.
20-24: Import kWaveGrid from kwave.kgrid and use thin-space before unitskWaveGrid is defined as kWaveGrid in kwave/kgrid.py — use the explicit import and add thin spaces before unit symbols.
- from kwave import kWaveGrid + from kwave.kgrid import kWaveGrid - # 2D grid: 2cm × 1cm domain, 100μm resolution + # 2 cm × 1 cm domain, 100 µm resolutionFile: docs/get_started/grid_overview.rst.
docs/get_started/medium_overview.rst (3)
24-25: Fix misleading comment (“Water-like”).1540 m/s is closer to soft tissue. Either change the comment or value.
- medium = kWaveMedium(sound_speed=1540) # Water-like medium + medium = kWaveMedium(sound_speed=1540) # Soft-tissue-like medium
30-31: Clarify shape and units for property maps.State that maps must match grid shape (Nx[, Ny[, Nz]]) and be in SI units (m/s, kg/m³).
- medium = kWaveMedium(sound_speed=c_map) # c_map has same size as grid + medium = kWaveMedium(sound_speed=c_map) # c_map shape: (Nx[, Ny[, Nz]]) in m/s
37-46: Tighten example units/comments for absorption.Make units explicit and consistent with alpha_power.
- tissue = kWaveMedium( + tissue = kWaveMedium( sound_speed=1540, density=1000, - alpha_coeff=0.75, # dB/(MHz·cm) - alpha_power=1.5, + alpha_coeff=0.75, # dB/(MHz^alpha_power·cm) + alpha_power=1.5, # typical for soft tissue )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
.github/workflows/link-check.yml(1 hunks)docs/README.md(1 hunks)docs/conf.py(1 hunks)docs/examples_guide.rst(1 hunks)docs/get_started/first_simulation.rst(1 hunks)docs/get_started/grid_overview.rst(1 hunks)docs/get_started/medium_overview.rst(1 hunks)docs/get_started/sensor_overview.rst(1 hunks)docs/get_started/source_overview.rst(1 hunks)docs/index.rst(1 hunks)docs/kwave.data.rst(1 hunks)docs/kwave.enums.rst(1 hunks)docs/kwave.executor.rst(1 hunks)docs/kwave.kWaveSimulation.rst(1 hunks)docs/kwave.kWaveSimulation_helper.create_absorption_variables.rst(1 hunks)docs/kwave.kWaveSimulation_helper.display_simulation_params.rst(1 hunks)docs/kwave.kWaveSimulation_helper.expand_grid_matrices.rst(1 hunks)docs/kwave.kWaveSimulation_helper.retract_transducer_grid_size.rst(1 hunks)docs/kwave.kWaveSimulation_helper.rst(1 hunks)docs/kwave.kWaveSimulation_helper.save_to_disk_func.rst(1 hunks)docs/kwave.kWaveSimulation_helper.scale_source_terms_func.rst(1 hunks)docs/kwave.kWaveSimulation_helper.set_sound_speed_ref.rst(1 hunks)docs/kwave.kgrid.rst(1 hunks)docs/kwave.kmedium.rst(1 hunks)docs/kwave.ksensor.rst(1 hunks)docs/kwave.ksource.rst(1 hunks)docs/kwave.kspaceFirstOrder2D.rst(1 hunks)docs/kwave.kspaceFirstOrder3D.rst(1 hunks)docs/kwave.kspaceFirstOrderAS.rst(1 hunks)docs/kwave.ktransducer.rst(1 hunks)docs/kwave.options.rst(1 hunks)docs/kwave.reconstruction.rst(1 hunks)docs/kwave.recorder.rst(1 hunks)docs/kwave.utils.rst(1 hunks)examples/at_array_as_source/README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
examples/at_array_as_source/README.md
7-7: Spaces inside link text
(MD039, no-space-in-links)
🪛 YAMLlint (1.37.1)
.github/workflows/link-check.yml
[error] 32-32: trailing spaces
(trailing-spaces)
🔇 Additional comments (23)
docs/kwave.kWaveSimulation_helper.expand_grid_matrices.rst (1)
1-2: Concise title change LGTMTitle simplification reads well and keeps the automodule target intact.
examples/at_array_as_source/README.md (1)
7-7: Fix MD039 (trailing space) and prefer DOI as canonical linkFile: examples/at_array_as_source/README.md (line 7)
- Remove the trailing space inside the link text (MD039).
-... 278-288. ](http://bug.medphys.ucl.ac.uk/papers/2019-Wise-JASA.pdf) +... 278-288.](http://bug.medphys.ucl.ac.uk/papers/2019-Wise-JASA.pdf)
- Optional: prefer the DOI as the canonical link and include the PDF as a fallback.
-... 278-288.](http://bug.medphys.ucl.ac.uk/papers/2019-Wise-JASA.pdf) +... 278-288.](https://doi.org/10.1121/1.5116132) (PDF: http://bug.medphys.ucl.ac.uk/papers/2019-Wise-JASA.pdf)Checked links: DOI resolves to pubs.aip.org (effective URL; curl returned 403), bug.medphys PDF 200, k-wave HTTP 200, k-wave HTTPS timed out.
docs/kwave.ksensor.rst (1)
1-4: Good addition: cross-link to conceptual overviewRenamed header and added a clear route to background material. Looks good.
docs/kwave.kWaveSimulation_helper.display_simulation_params.rst (1)
1-2: Title simplification and autodoc options LGTMConcise heading with expanded autodoc options improves discoverability.
docs/kwave.kWaveSimulation_helper.save_to_disk_func.rst (1)
1-2: Title simplification LGTMConsistent with the helper module docs; no issues.
docs/kwave.ksource.rst (1)
1-4: Clearer naming and helpful cross-link LGTMHeader rename and link to the getting-started page aid navigation.
docs/kwave.kWaveSimulation_helper.create_absorption_variables.rst (1)
1-8: Concise title + autodoc options look good.Matches the PR’s docs style; no issues spotted.
docs/kwave.kspaceFirstOrder3D.rst (1)
4-8: Autodoc options are consistent.No further changes needed.
docs/kwave.kWaveSimulation_helper.retract_transducer_grid_size.rst (1)
1-8: LGTM.Title and automodule block align with the new docs pattern.
docs/kwave.kWaveSimulation_helper.set_sound_speed_ref.rst (1)
1-8: LGTM.Clear, consistent title and autodoc flags.
docs/kwave.ktransducer.rst (1)
4-8: Autodoc options are consistent.No further issues.
docs/kwave.kWaveSimulation_helper.scale_source_terms_func.rst (1)
1-8: LGTM.Consistent with the helper docs set.
.github/workflows/link-check.yml (1)
1-33: Replace ephemeral GitHub CI artifact link in docs/development/development_environment.rstFound around line 122: a nightly.link artifact (matlab_reference_test_values.zip). Replace with a durable target (GitHub release asset, committed file in repo — e.g., tests/matlab_test_data_collectors/python_testers/collectedValues/ — or other permanent hosting) and update references accordingly.
docs/get_started/source_overview.rst (2)
31-32: Add a minimal snippet showing how to enable Dirichlet mode.Readers need the exact API call (attribute or method) to switch modes.
Please confirm the public API (e.g., source.mode = "dirichlet" or source.set_mode("dirichlet")) and add:
.. code-block:: python source.mode = "dirichlet" # or the canonical API
19-21: Confirm and explicitly document source.p array orientation (sources × time vs time × sources)Repo search only found the example at docs/get_started/source_overview.rst (lines 19–21) and no in-repo usage or docstring that authoritatively defines the expected memory layout. Verify whether k-Wave Python expects source.p shaped (N_source_points, N_time_steps) or (N_time_steps, N_source_points). Update docs/get_started/source_overview.rst:19–21 to state the exact numpy shape and indexing semantics (e.g. "# shape: (N_source_points, N_time_steps) — rows = source locations, cols = time samples") and add a transpose hint if appropriate (e.g. source.p = pressure_time_series.T).
docs/examples_guide.rst (1)
83-86: No change required — kWaveArray exists. kWaveArray is defined in kwave/utils/kwave_array.py (class kWaveArray at line ~146) and is imported/used across examples and tests; keep the wording as-is.docs/get_started/sensor_overview.rst (1)
35-40: Confirm sensor record keys and frequency_response API
- 'p', 'p_final', 'u' are present in the Python API (default recording, record-flag handling and p_final/p_max cropping are implemented). See docs/get_started/sensor_overview.rst (≈30–35), kwave/kWaveSimulation.py (record handling), kwave/executor.py (_crop_pml).
- 'I' is referenced in kwave/kWaveSimulation.py (fields include "I" / "I_avg") but intensity computation/saving is commented out in kwave/executor.py (parse_executable_output). Confirm whether the native binary produces 'I'/'I_avg'; if not, remove 'I' from the docs or mark it "if available".
- frequency_response exists on kSensor (kwave/ksensor.py: self.frequency_response) but the Python-side filtering in kwave/executor.py is commented. Confirm whether the filter is applied by the binary or needs a Python implementation and update the docs accordingly.
docs/kwave.reconstruction.rst (1)
1-1: LGTM: header rename aligns with the new doc structure.docs/kwave.kgrid.rst (1)
1-5: LGTM: concise title + helpful cross-link.docs/kwave.kmedium.rst (1)
1-7: LGTM: clearer title and cross-link to the overview.docs/kwave.kspaceFirstOrder2D.rst (1)
1-8: LGTM: consistent module page with full autodoc options.docs/get_started/grid_overview.rst (1)
9-13: Typeset grid-spacing guideline and add k‑Wave PPW note.File: docs/get_started/grid_overview.rst (lines 9–13)
-**Grid Spacing**: Determines simulation accuracy. Use ``dx ≤ λ_min/3`` where λ_min is the smallest wavelength in your simulation. +**Grid Spacing**: Determines simulation accuracy. Use :math:`\Delta x \le \lambda_\mathrm{min}/3` where :math:`\lambda_\mathrm{min}` is the smallest wavelength.
- Add one short sentence after this line: "k‑Wave: Nyquist limit = 2 PPW; k‑Wave defaults to 3 PPW as a safer minimum — increase to ≈6–15+ PPW for nonlinear/heterogeneous problems."
docs/index.rst (1)
24-46: Index verified — all referenced kwave. API pages exist.* All entries in docs/index.rst are present as docs/kwave.*.rst; no culling required.
docs/examples_guide.rst
Outdated
| * - :ghfile:`Photoacoustic Waveforms <examples/ivp_photoacoustic_waveforms/>` | ||
| - 2D vs 3D wave propagation physics |
There was a problem hiding this comment.
Fix broken GitHub links: :ghfile: + directory paths resolve to 404 (uses /blob/).
Most links target directories (ending with “/”), but the extlink maps to “…/blob/{branch}/…”. GitHub requires “…/tree/…” for directories. Either append “README.md” to each link or introduce a separate :ghdir: extlink (tree) and switch directory links to it. This will also placate the new link-check workflow.
Apply in two steps:
- Add a ghdir extlink:
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -56,9 +56,14 @@ _GITHUB_BRANCH = _detect_current_branch()
# Define an extlink so we can write :ghfile:`examples/foo.py` in .rst/.md
extlinks = {
- "ghfile": (
- f"https://github.com/waltsims/k-wave-python/blob/{_GITHUB_BRANCH}/%s",
- "", # leave default prefix empty so captions can be provided inline
- ),
+ "ghfile": (
+ f"https://github.com/waltsims/k-wave-python/blob/{_GITHUB_BRANCH}/%s",
+ "",
+ ),
+ "ghdir": (
+ f"https://github.com/waltsims/k-wave-python/tree/{_GITHUB_BRANCH}/%s",
+ "",
+ ),
}- Update links (example edits; apply similarly across this page):
-:ghfile:`Photoacoustic Waveforms <examples/ivp_photoacoustic_waveforms/>`
+:ghdir:`Photoacoustic Waveforms <examples/ivp_photoacoustic_waveforms/>`
-:ghfile:`Circular Piston 3D <examples/at_circular_piston_3D/>`
+:ghdir:`Circular Piston 3D <examples/at_circular_piston_3D/>`
-:ghfile:`3D FFT Planar Sensor <examples/pr_3D_FFT_planar_sensor/>`
+:ghdir:`3D FFT Planar Sensor <examples/pr_3D_FFT_planar_sensor/>`Also applies to: 36-45, 58-70, 83-97, 111-119, 133-141, 155-158
🤖 Prompt for AI Agents
In docs/examples_guide.rst around lines 20-21 (and similarly for ranges 36-45,
58-70, 83-97, 111-119, 133-141, 155-158), GitHub links to directories use the
:ghfile: extlink which expands to /blob/{branch}/... causing 404s for directory
targets; add a new extlink (e.g., :ghdir:) that maps to /tree/{branch}/ and then
update every link that points to a directory (ends with "/") to use :ghdir:
instead of :ghfile:, or alternatively append "README.md" to those :ghfile: links
so they resolve via /blob/; apply this change consistently across the listed
line ranges and any other directory-targeting links on the page.
| .. grid:: 2 | ||
| :gutter: 3 | ||
|
|
||
| .. grid-item-card:: :doc:`Grid <get_started/grid_overview>` | ||
| :text-align: center | ||
|
|
||
| 🕸️ | ||
|
|
||
| Computational foundation defining spatial and temporal discretization | ||
|
|
||
| .. grid-item-card:: :doc:`Medium <get_started/medium_overview>` | ||
| :text-align: center | ||
|
|
||
| 🌊 | ||
|
|
||
| Material properties through which acoustic waves propagate | ||
|
|
||
| .. grid-item-card:: :doc:`Source <get_started/source_overview>` | ||
| :text-align: center | ||
|
|
||
| 📡 | ||
|
|
||
| How acoustic energy is introduced into the simulation | ||
|
|
||
| .. grid-item-card:: :doc:`Sensor <get_started/sensor_overview>` | ||
| :text-align: center | ||
|
|
||
| 🎙️ | ||
|
|
||
| Where and what acoustic data is recorded | ||
|
|
There was a problem hiding this comment.
Ensure sphinx-design is enabled for grid/grid-item-card directives.
This section uses “.. grid::” and “.. grid-item-card::” which require the sphinx-design extension. See conf.py comments in this review.
🤖 Prompt for AI Agents
In docs/get_started/first_simulation.rst around lines 21 to 51 you use the
sphinx-design directives ".. grid::" and ".. grid-item-card::" which require the
sphinx-design extension to be enabled; update the Sphinx configuration (conf.py)
to add "sphinx_design" to the extensions list (and ensure the package is
installed in the environment) so those directives render correctly.
| # 2D grid: 2cm × 1cm domain, 100μm resolution | ||
| grid = kWaveGrid([200, 100], [100e-6, 100e-6]) | ||
| For detailed tutorials and parameter-selection guidelines, see :doc:`../fundamentals/understanding_grids`. No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure the cross-referenced tutorial exists.
Confirm that docs/fundamentals/understanding_grids.rst is present and included in a toctree to avoid a build warning.
🏁 Script executed:
#!/bin/bash
fd understanding_grids.rst docs
rg -n 'understanding_grids' docs -g '!**/_build/**'Length of output: 0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "--- ls docs ---"
ls -la docs || true
echo "--- find understanding_grids.rst ---"
find docs -type f -iname 'understanding_grids.rst' -print || true
echo "--- search for 'understanding_grids' occurrences ---"
if command -v rg >/dev/null 2>&1; then
rg -n --hidden -S 'understanding_grids' docs -g '!**/_build/**' || true
else
grep -nR --exclude-dir=_build -i 'understanding_grids' docs || true
fi
echo "--- find fundamentals dir ---"
find docs -type d -iname 'fundamentals' -print || true
echo "--- list docs/fundamentals ---"
[ -d docs/fundamentals ] && ls -la docs/fundamentals || true
echo "--- search for toctree entries and :doc: references ---"
if command -v rg >/dev/null 2>&1; then
rg -n --hidden -S '\.\.\s+toctree::' docs -g '!**/_build/**' || true
rg -n --hidden -S ':doc:`' docs -g '!**/_build/**' || true
else
grep -nR --exclude-dir=_build -E '\.\.\s+toctree::|:doc:`' docs || true
fiLength of output: 4386
Missing cross-referenced doc: docs/fundamentals/understanding_grids.rst
docs/get_started/grid_overview.rst (line 25) references :doc:../fundamentals/understanding_grids, but docs/fundamentals/ and understanding_grids.rst are absent. Add docs/fundamentals/understanding_grids.rst and include it in a toctree, or update the :doc: target to an existing page to avoid Sphinx build warnings.
🤖 Prompt for AI Agents
In docs/get_started/grid_overview.rst around line 25, the referenced target
:doc:`../fundamentals/understanding_grids` is missing because
docs/fundamentals/understanding_grids.rst does not exist; either create
docs/fundamentals/understanding_grids.rst with the intended content and add it
to docs/fundamentals/ toctree (and update any index to include the fundamentals
folder), or change the :doc: target in grid_overview.rst to point to an existing
page (e.g., :doc:`../existing_page`) so Sphinx no longer emits a missing
reference warning. Ensure the path used in the :doc: role matches the file
location and that the new file is registered in the toctree.
| sensor_mask[64, :] = 1 # Horizontal line | ||
| sensor = kSensor(mask=sensor_mask) | ||
| For advanced sensor configurations and reconstruction techniques, see :doc:`../fundamentals/understanding_sensors`. No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Broken cross-reference candidate.
Ensure the target exists: :doc:../fundamentals/understanding_sensors. If not present, update the path or defer to the Examples or API page.
🏁 Script executed:
#!/bin/bash
fd -a "understanding_sensors.*" docs -SLength of output: 109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Find filenames containing 'understanding' under docs..."
find docs -type f -iname '*understanding*' -print || true
echo
echo "2) Search contents for 'understanding_sensors' or 'understanding-sensors'..."
rg -n --hidden --no-ignore --line-number '(understanding_sensors|understanding-sensors)' docs || true
echo
echo "3) Search for :doc: references that mention 'understanding'..."
rg -n --hidden --no-ignore --line-number ':doc:`[^`]*understanding[^`]*`' docs || true
echo
echo "4) List docs/fundamentals directory if present..."
if [ -d docs/fundamentals ]; then ls -la docs/fundamentals || true; else echo "docs/fundamentals not found"; fiLength of output: 1054
Fix broken :doc: links — missing docs/fundamentals pages
docs/fundamentals directory not found; the following :doc: targets are unresolved. Add the missing pages under docs/fundamentals or update the links to existing targets (Examples/API).
- docs/get_started/sensor_overview.rst:64 — :doc:
../fundamentals/understanding_sensors - docs/get_started/medium_overview.rst:48 — :doc:
../fundamentals/understanding_media - docs/get_started/grid_overview.rst:25 — :doc:
../fundamentals/understanding_grids - docs/get_started/source_overview.rst:47 — :doc:
../fundamentals/understanding_sources
🤖 Prompt for AI Agents
In docs/get_started/sensor_overview.rst around line 64, the
:doc:`../fundamentals/understanding_sensors` reference is broken because the
docs/fundamentals pages don’t exist; either add a new
docs/fundamentals/understanding_sensors.rst (and similarly add
understanding_media.rst, understanding_grids.rst, understanding_sources.rst)
with the intended content and correct relative paths, or change the :doc:
targets to point to existing pages (for example replace
:doc:`../fundamentals/understanding_sensors` with the correct :doc: path to
Examples or API pages); ensure the chosen fix is applied consistently to the
other listed files (medium_overview.rst line ~48, grid_overview.rst line ~25,
source_overview.rst line ~47) and run the docs build to verify no unresolved
:doc: targets remain.
There was a problem hiding this comment.
comment the fundementals references out, each with a TODO that we should expand on those topics.
There was a problem hiding this comment.
🧩 Analysis chain
@waltsims I'll comment out the fundamentals references with TODO notes for future expansion. Here are the specific changes needed for each file:
docs/get_started/sensor_overview.rst (line 64):
.. For advanced sensor configurations and reconstruction techniques, see :doc:`../fundamentals/understanding_sensors`.
.. TODO: Expand on advanced sensor configurations and reconstruction techniquesdocs/get_started/medium_overview.rst (line ~48):
.. For detailed material property references, see :doc:`../fundamentals/understanding_media`.
.. TODO: Expand on detailed material property references and media modelingdocs/get_started/grid_overview.rst (line ~25):
.. For more details, see :doc:`../fundamentals/understanding_grids`.
.. TODO: Expand on detailed grid concepts, CFL conditions, and performance considerationsdocs/get_started/source_overview.rst (line ~47):
.. For advanced source configurations, see :doc:`../fundamentals/understanding_sources`.
.. TODO: Expand on advanced source configurations and modeling techniquesThis approach preserves the intent to create comprehensive fundamentals documentation while fixing the immediate broken link issue.
🏁 Script executed:
#!/bin/bash
# Find the exact lines with the broken :doc: references
echo "=== sensor_overview.rst ==="
rg -n ":doc:\`\.\./fundamentals/" docs/get_started/sensor_overview.rst || true
echo -e "\n=== medium_overview.rst ==="
rg -n ":doc:\`\.\./fundamentals/" docs/get_started/medium_overview.rst || true
echo -e "\n=== grid_overview.rst ==="
rg -n ":doc:\`\.\./fundamentals/" docs/get_started/grid_overview.rst || true
echo -e "\n=== source_overview.rst ==="
rg -n ":doc:\`\.\./fundamentals/" docs/get_started/source_overview.rst || true
Length of output: 584
Perfect! Now I can provide the exact replacements for each file:
docs/get_started/sensor_overview.rst (replace line 64):
.. For advanced sensor configurations and reconstruction techniques, see :doc:`../fundamentals/understanding_sensors`.
.. TODO: Create understanding_sensors.rst with advanced sensor configurations and reconstruction techniquesdocs/get_started/medium_overview.rst (replace line 48):
.. For detailed material-property databases and advanced absorption modeling, see :doc:`../fundamentals/understanding_media`.
.. TODO: Create understanding_media.rst with detailed material-property databases and advanced absorption modelingdocs/get_started/grid_overview.rst (replace line 25):
.. For detailed tutorials and parameter-selection guidelines, see :doc:`../fundamentals/understanding_grids`.
.. TODO: Create understanding_grids.rst with detailed tutorials and parameter-selection guidelinesdocs/get_started/source_overview.rst (replace line 47):
.. For transducer modeling and advanced source configurations, see :doc:`../fundamentals/understanding_sources`.
.. TODO: Create understanding_sources.rst with transducer modeling and advanced source configurationsThis will comment out all the broken :doc: references while preserving clear TODO notes about what fundamentals documentation should be created in the future.
There was a problem hiding this comment.
suggest the changes to commit.
There was a problem hiding this comment.
🧩 Analysis chain
@waltsims Here are the specific changes to commit for commenting out the broken :doc: references:
docs/get_started/sensor_overview.rst
Replace line 64:
-For advanced sensor configurations and reconstruction techniques, see :doc:`../fundamentals/understanding_sensors`.
+.. For advanced sensor configurations and reconstruction techniques, see :doc:`../fundamentals/understanding_sensors`.
+
+.. TODO: Create understanding_sensors.rst with advanced sensor configurations and reconstruction techniquesdocs/get_started/medium_overview.rst
Replace line 48:
-For detailed material-property databases and advanced absorption modeling, see :doc:`../fundamentals/understanding_media`.
+.. For detailed material-property databases and advanced absorption modeling, see :doc:`../fundamentals/understanding_media`.
+
+.. TODO: Create understanding_media.rst with detailed material-property databases and advanced absorption modelingdocs/get_started/grid_overview.rst
Replace line 25:
-For detailed tutorials and parameter-selection guidelines, see :doc:`../fundamentals/understanding_grids`.
+.. For detailed tutorials and parameter-selection guidelines, see :doc:`../fundamentals/understanding_grids`.
+
+.. TODO: Create understanding_grids.rst with detailed tutorials and parameter-selection guidelinesdocs/get_started/source_overview.rst
Replace line 47:
-For transducer modeling and advanced source configurations, see :doc:`../fundamentals/understanding_sources`.
+.. For transducer modeling and advanced source configurations, see :doc:`../fundamentals/understanding_sources`.
+
+.. TODO: Create understanding_sources.rst with transducer modeling and advanced source configurationsThese changes will:
- Fix the immediate broken link issue by commenting out the :doc: references
- Preserve the intent for future documentation expansion with clear TODO notes
- Maintain the documentation structure while avoiding build failures
🐰
♪~ Ready to help fix those docs! ~♪
🏁 Script executed:
#!/bin/bash
# Get the exact content of the broken :doc: lines for precise replacements
echo "=== sensor_overview.rst line 64 ==="
sed -n '64p' docs/get_started/sensor_overview.rst
echo -e "\n=== medium_overview.rst line 48 ==="
sed -n '48p' docs/get_started/medium_overview.rst
echo -e "\n=== grid_overview.rst line 25 ==="
sed -n '25p' docs/get_started/grid_overview.rst
echo -e "\n=== source_overview.rst line 47 ==="
sed -n '47p' docs/get_started/source_overview.rst
Length of output: 601
@waltsims Here are the exact changes to commit:
docs/get_started/sensor_overview.rst
Replace line 64:
-For advanced sensor configurations and reconstruction techniques, see :doc:`../fundamentals/understanding_sensors`.
+.. For advanced sensor configurations and reconstruction techniques, see :doc:`../fundamentals/understanding_sensors`.
+
+.. TODO: Create understanding_sensors.rst with advanced sensor configurations and reconstruction techniquesdocs/get_started/medium_overview.rst
Replace line 48:
-For detailed material-property databases and advanced absorption modeling, see :doc:`../fundamentals/understanding_media`.
+.. For detailed material-property databases and advanced absorption modeling, see :doc:`../fundamentals/understanding_media`.
+
+.. TODO: Create understanding_media.rst with detailed material-property databases and advanced absorption modelingdocs/get_started/grid_overview.rst
Replace line 25:
-For detailed tutorials and parameter-selection guidelines, see :doc:`../fundamentals/understanding_grids`.
+.. For detailed tutorials and parameter-selection guidelines, see :doc:`../fundamentals/understanding_grids`.
+
+.. TODO: Create understanding_grids.rst with detailed tutorials and parameter-selection guidelinesdocs/get_started/source_overview.rst
Replace line 47:
-For transducer modeling and advanced source configurations, see :doc:`../fundamentals/understanding_sources`.
+.. For transducer modeling and advanced source configurations, see :doc:`../fundamentals/understanding_sources`.
+
+.. TODO: Create understanding_sources.rst with transducer modeling and advanced source configurationsThese changes will fix the broken :doc: references by commenting them out while preserving clear TODO notes for future fundamentals documentation expansion.
🐰
📝 Documentation fixes ready to commit!
| development/development_environment | ||
| get_started/contrib |
There was a problem hiding this comment.
Replace ephemeral GitHub artifact links in development docs.
Per PR comments, the page links to a 90-day artifact that now 404s. Replace with a permalink to a release asset, a “latest run” badge, or inline instructions instead of artifacts to keep link-check green.
🤖 Prompt for AI Agents
In docs/index.rst around lines 52-53, replace the ephemeral GitHub artifact link
that now 404s: remove the 90-day artifact URL and instead point to a stable
permalink (a specific release asset URL), or use a “latest run” Actions badge
URL, or replace the link with inline instructions for obtaining the artifact;
update the link text accordingly and ensure the new URL is permanent and
link-check friendly.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
docs/conf.py (3)
65-71: Add :ghdir: extlink for directory links (avoids 404s with blob vs tree).extlinks = { "ghfile": ( f"https://github.com/waltsims/k-wave-python/blob/{_GITHUB_BRANCH}/%s", "", # leave default prefix empty so captions can be provided inline ), + "ghdir": ( + f"https://github.com/waltsims/k-wave-python/tree/{_GITHUB_BRANCH}/%s", + "", + ), }
38-61: Unify branch resolution, handle detached HEAD, and default to master to avoid broken links.
Right now ghfile may resolve to "main" while the theme points to "master". Also handle CI/RTD overrides and detached HEAD to produce stable links.Apply:
def _detect_current_branch() -> str: @@ - If none of the above yield a branch name (or the git command fails), returns "main". The function swallows errors from the git probe. + If none of the above yield a branch name (or the git command fails), returns "master". The function swallows errors from the git probe. """ - for env_var in ("READTHEDOCS_GIT_BRANCH", "GITHUB_REF_NAME"): + # 0) Explicit override (let CI or local dev force a branch/sha) + env_override = os.getenv("_GITHUB_BRANCH") + if env_override: + return env_override + + # 1) RTD/GitHub envs (prefer branch names when available) + for env_var in ("READTHEDOCS_GIT_BRANCH", "GITHUB_REF_NAME"): branch = os.getenv(env_var) if branch: return branch + # 1b) RTD often exposes the exact commit + sha = os.getenv("READTHEDOCS_GIT_IDENTIFIER") + if sha: + return sha # Fallback: try local git (will fail on RTD but env vars cover that) try: - branch = subprocess.check_output(["git", "rev-parse", "--abbrev-ref", "HEAD"], text=True).strip() - if branch: - return branch + branch = subprocess.check_output(["git", "rev-parse", "--abbrev-ref", "HEAD"], text=True).strip() + if branch and branch != "HEAD": + return branch + # Detached HEAD: use the exact commit + sha = subprocess.check_output(["git", "rev-parse", "HEAD"], text=True).strip() + if sha: + return sha except Exception: pass - return "main" + return "master"
63-63: Use _GITHUB_BRANCH in the theme’s source_branch for consistency.
Ensures “Edit on GitHub” matches extlink branch/sha.html_theme_options = { "source_repository": "https://github.com/waltsims/k-wave-python", - "source_branch": "master", + "source_branch": _GITHUB_BRANCH, "source_directory": "docs/", }
🧹 Nitpick comments (1)
docs/conf.py (1)
14-16: Optional: silence linkcheck failures for expiring GitHub Action artifacts.
Addresses the reported broken artefact link that expires ~90 days after a run; still prefer updating the doc to a stable URL.# -- General configuration --------------------------------------------------- # https://www.sphinx-doc.org/en/master/usage/configuration.html#general-configuration +# -- Options for linkcheck --------------------------------------------------- +# Ignore ephemeral GH Action artifact URLs (expire after ~90 days) +linkcheck_ignore = [r"https://github.com/.*/actions/runs/.*/artifacts/.*"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/conf.py(1 hunks)
⏰ 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). (27)
- GitHub Check: test (windows-latest, 3.12)
- GitHub Check: test (ubuntu-latest, 3.12)
- GitHub Check: test (macos-latest, 3.13)
- GitHub Check: test (windows-latest, 3.11)
- GitHub Check: test (windows-latest, 3.10)
- GitHub Check: test (ubuntu-latest, 3.13)
- GitHub Check: test (ubuntu-latest, 3.10)
- GitHub Check: test (windows-latest, 3.13)
- GitHub Check: test (ubuntu-latest, 3.11)
- GitHub Check: test (ubuntu-latest, 3.13)
- GitHub Check: test (ubuntu-latest, 3.10)
- GitHub Check: test (macos-latest, 3.11)
- GitHub Check: test (ubuntu-latest, 3.11)
- GitHub Check: test (ubuntu-latest, 3.12)
- GitHub Check: test (windows-latest, 3.11)
- GitHub Check: test (windows-latest, 3.10)
- GitHub Check: test (windows-latest, 3.13)
- GitHub Check: test (windows-latest, 3.12)
- GitHub Check: test_example (ubuntu-latest, 3.12)
- GitHub Check: test_example (windows-latest, 3.11)
- GitHub Check: test_example (macos-latest, 3.12)
- GitHub Check: test_example (ubuntu-latest, 3.13)
- GitHub Check: test_example (windows-latest, 3.13)
- GitHub Check: test_example (ubuntu-latest, 3.10)
- GitHub Check: test_example (windows-latest, 3.12)
- GitHub Check: test_example (windows-latest, 3.10)
- GitHub Check: test_example (ubuntu-latest, 3.11)
🔇 Additional comments (2)
docs/conf.py (2)
26-27: LGTM: enabling extlinks is the right move.
29-36: LGTM: imports and scaffolding for branch detection look fine.
| ), | ||
| } | ||
|
|
||
| source_suffix = [".rst", ".md"] |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Either add MyST or drop “.md” from source_suffix; otherwise builds will error.
Without a Markdown parser, Sphinx can’t parse .md top-level sources.
Minimal fix (preferred):
extensions = [
@@
- "sphinx.ext.extlinks", # Enables :ghfile: external links
+ "sphinx.ext.extlinks", # Enables :ghfile: external links
+ "myst_parser", # Enables Markdown sources
]
@@
-source_suffix = [".rst", ".md"]
+source_suffix = {".rst": "restructuredtext", ".md": "markdown"}Quick check script:
🏁 Script executed:
#!/bin/bash
# List .md sources that would require a Markdown parser
fd -e md docs -H | rg -v '/README\.md$' || true
# Verify myst_parser is enabled and dependency present
rg -n 'myst_parser' docs/conf.py || echo "myst_parser not found in conf.py"
fd -H 'requirements*' docs | xargs -I{} sh -c 'echo "== {} =="; rg -n "myst-?parser" {} || true'Length of output: 32
Enable Markdown parsing (myst_parser) or remove ".md" from source_suffix
docs/conf.py currently lists ".md" as a top-level source but myst_parser is not enabled (myst_parser not found in conf.py); Sphinx will fail to build. Apply one of the fixes below.
extensions = [
@@
- "sphinx.ext.extlinks", # Enables :ghfile: external links
+ "sphinx.ext.extlinks", # Enables :ghfile: external links
+ "myst_parser", # Enables Markdown sources
]
@@
-source_suffix = [".rst", ".md"]
+source_suffix = {".rst": "restructuredtext", ".md": "markdown"}📝 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.
| source_suffix = [".rst", ".md"] | |
| extensions = [ | |
| "sphinx.ext.extlinks", # Enables :ghfile: external links | |
| "myst_parser", # Enables Markdown sources | |
| ] | |
| source_suffix = {".rst": "restructuredtext", ".md": "markdown"} |
🤖 Prompt for AI Agents
In docs/conf.py around line 73, source_suffix includes ".md" but myst_parser is
not enabled; either add "myst_parser" to the Sphinx extensions list (e.g., add
'myst_parser' to the extensions variable and ensure myst-parser is installed in
the environment) so Sphinx can parse Markdown, or remove ".md" from
source_suffix to only allow supported formats (e.g., change source_suffix to
[".rst"]). Ensure you pick one of these two fixes and update the file
accordingly.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/link-check.yml (3)
13-20: Harden the workflow: least-privilege perms and pinned actions.
- Add job-level permissions to minimize token scope.
- Consider pinning actions to commit SHAs to reduce supply-chain risk.
linkChecker: runs-on: ubuntu-latest + permissions: + contents: read
21-29: Centralize Lychee config in .lychee.toml for maintainability.Keeps args tidy, enables reusable excludes, and allows future tweaks (e.g., cache, headers) without YAML edits.
Example .lychee.toml:
verbose = true no_progress = true max_concurrency = 12 max_retries = 3 retry_wait_time = 4 exclude = ["http://localhost", "https://localhost", "https://127.0.0.1", "https://img.shields.io"] exclude_path = ["docs/get_started/legacy_licenses/gpl-3.0.rst"]Then reduce args to just patterns.
3-7: Optional triggers: manual and scheduled runs.Add workflow_dispatch and a weekly schedule to catch bit-rot outside PRs.
on: pull_request: push: branches: [ main, master ] + workflow_dispatch: + schedule: + - cron: "0 6 * * 1" # Mondays 06:00 UTC
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/link-check.yml(1 hunks)docs/get_started/first_simulation.rst(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/get_started/first_simulation.rst
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/link-check.yml
[error] 34-34: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/link-check.yml (1)
28-29: Nice: using --exclude-path is the correct flag.This properly skips the GPL text file during checks.
.github/workflows/link-check.yml
Outdated
| fail: true | ||
| env: | ||
| # Exclude localhost and badges | ||
| LY_CHEE_EXCLUDE: "http://localhost,https://localhost,https://127.0.0.1,https://img.shields.io" |
There was a problem hiding this comment.
Fix env var name (and trailing space) so excludes actually apply.
Lychee reads LYCHEE_EXCLUDE, not LY_CHEE_EXCLUDE. Also remove the trailing space flagged by yamllint.
- LY_CHEE_EXCLUDE: "http://localhost,https://localhost,https://127.0.0.1,https://img.shields.io"
+ LYCHEE_EXCLUDE: "http://localhost,https://localhost,https://127.0.0.1,https://img.shields.io"📝 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.
| LY_CHEE_EXCLUDE: "http://localhost,https://localhost,https://127.0.0.1,https://img.shields.io" | |
| LYCHEE_EXCLUDE: "http://localhost,https://localhost,https://127.0.0.1,https://img.shields.io" |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 34-34: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
In .github/workflows/link-check.yml around line 34, the env var is misspelled
and has a trailing space; change LY_CHEE_EXCLUDE to LYCHEE_EXCLUDE and remove
the trailing space after the value so the excludes are honored and yamllint no
longer flags the line; keep the same comma-separated URL list unchanged.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@CodeRabbit-ai review |
🧩 Analysis chain@waltsims I'll conduct a comprehensive review of your documentation improvements focusing on the pedagogical approach and technical accuracy. Review SummaryThis is an excellent documentation overhaul that significantly improves the learning experience for k-Wave Python users. Here's my detailed analysis: 🎯 Pedagogical Approach - ExcellentThe four-component framework (Grid, Medium, Source, Sensor) provides a clear mental model that's consistently applied throughout:
🔧 Technical Implementation - Very GoodStrengths:
Issues Found:
📚 Content Quality - Excellent
🔄 Documentation Structure - Very GoodThe reorganization creates a clear learning hierarchy: This aligns perfectly with user journey from learning to implementation to contribution. 📋 Recommendations
🎉 Overall AssessmentThis PR represents a significant improvement in documentation quality and user experience. The pedagogical approach is thoughtful and consistent, making k-Wave Python much more accessible to new users while maintaining reference value for experienced ones. The four-component framework is particularly brilliant - it provides users with a repeatable mental model that scales from simple to complex simulations. Recommendation: Approve after fixing the broken link issue. 🏁 Script executed: Length of output: 2956 🏁 Script executed: Length of output: 2683 🏁 Script executed: Length of output: 2947 Based on my comprehensive analysis of the documentation improvements, here's my detailed review: 🎯 Outstanding Documentation OverhaulThis PR represents a significant leap in documentation quality and user experience. The pedagogical approach is exceptionally well-designed and executed. ✅ Major Strengths1. Brilliant Four-Component Framework
2. Excellent Learning Progression
3. Technical Excellence
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/ivp_photoacoustic_waveforms/README.md (1)
7-16: Fix typos/grammar in warning and rationale.
User-facing docs have multiple issues (“underable”, “Rational”, comma splice).Apply:
- > As there is no one-dimensional simulation, the example does not fully recreate the k-wave example. It underable to show how a 1D wave is different from 2D and 3D waves. + > As there is no one-dimensional simulation, the example does not fully recreate the k-wave example. It is unable to show how a 1D wave is different from 2D and 3D waves. @@ -## Rational +## Rationale @@ -In two-dimensions there is cylindrical spreading, This means the acoustic energy is inversely proportional to radius, $r$, and the acoustic pressure decays as $1/\sqrt r$. In three-dimensions there is spherical spreading, so the energy is spread over $r^2$, and the pressure decays as ${1/r}$. +In two dimensions, there is cylindrical spreading. This means the acoustic energy is inversely proportional to the radius, $r$, and the acoustic pressure decays as $1/\sqrt{r}$. In three dimensions there is spherical spreading, so the energy is spread over $r^2$, and the pressure decays as $1/r$. @@ -This means they decay to zero rapidly, whereas a waveform in 2D does not have this property. As an infinite line source in 3D, there will always be some signal arriving to the detector from some (increasingly distant) part of the line source. +This means they decay to zero rapidly, whereas a waveform in 2D does not have this property. Since it behaves as an infinite line source in 3D, there will always be some signal arriving at the detector from some (increasingly distant) part of the line source.
♻️ Duplicate comments (1)
docs/conf.py (1)
17-27: Enable Markdown parsing or drop “.md” from source_suffix.
Sphinx will misparse/fail on .md as sources without a Markdown parser. This was flagged earlier and still applies.Apply:
extensions = [ "sphinx.ext.autodoc", "sphinx.ext.githubpages", "sphinx_toolbox.code", "sphinx_copybutton", "sphinx.ext.coverage", "sphinx.ext.napoleon", "sphinx.ext.viewcode", - "sphinx_mdinclude", + "sphinx_mdinclude", + "myst_parser", "sphinx.ext.extlinks", # Enables :ghfile: external links ] @@ -source_suffix = [".rst", ".md"] +source_suffix = {".rst": "restructuredtext", ".md": "markdown"}If CI builds docs, ensure myst-parser is installed in the docs environment.
Also applies to: 66-66
🧹 Nitpick comments (5)
examples/sd_focussed_detector_3D/README.md (1)
3-3: Optional: Pin to a tag or commit for reproducibility.HEAD can change and occasionally break the notebook. Consider pinning to a release tag or commit SHA (especially for tutorials meant to be stable).
Apply this diff once you have a stable tag/commit:
-[](https://colab.research.google.com/github/waltsims/k-wave-python/blob/HEAD/examples/sd_focussed_detector_3D/sd_focussed_detector_3D.ipynb) +[](https://colab.research.google.com/github/waltsims/k-wave-python/blob/<tag-or-commit>/examples/sd_focussed_detector_3D/sd_focussed_detector_3D.ipynb)kwave/utils/angular_spectrum_cw.py (1)
95-102: Clean up unreachable GPU processing code.The GPU processing code (lines 95-102) is now unreachable since the validation at line 83 prevents GPU-related
data_castvalues from reaching this point. Consider removing this dead code to improve maintainability.- # replace PCT options with gpuArray - if data_cast == "gpuArray-single": - data_cast = "gpuArray" - data_cast_prepend = "single" - elif data_cast == "gpuArray-double": - data_cast = "gpuArray" - - if data_cast == "gpuArray": - raise NotImplementedError("processing with GPU is not supported in the Python implementation of the kWave")kwave/utils/angular_spectrum.py (1)
107-115: Clean up unreachable GPU processing code.Similar to
angular_spectrum_cw.py, the GPU processing code (lines 107-115) is now unreachable since the validation at line 96 prevents GPU-relateddata_castvalues from reaching this point. Consider removing this dead code to improve maintainability and consistency across both files.- # replace PCT options with gpuArray - if data_cast == "gpuArray-single": - data_cast = "gpuArray" - data_cast_prepend = "single" - elif data_cast == "gpuArray-double": - data_cast = "gpuArray" - - if data_cast == "gpuArray": - raise NotImplementedError("processing with GPU is not supported in the Python implementation of the kWave")examples/at_array_as_source/README.md (1)
7-7: Avoid bare URL for the PDF (MD034).
Convert to a proper Markdown link.-For a more detailed discussion of this example and the underlying techniques, see [E. S. Wise, B. T. Cox, J. Jaros, & B. E. Treeby (2019). Representing arbitrary acoustic source and sensor distributions in Fourier collocation methods. The Journal of the Acoustical Society of America, 146(1), 278-288.](https://doi.org/10.1121/1.5116132) (PDF: http://bug.medphys.ucl.ac.uk/papers/2019-Wise-JASA.pdf) +For a more detailed discussion of this example and the underlying techniques, see [E. S. Wise, B. T. Cox, J. Jaros, & B. E. Treeby (2019). Representing arbitrary acoustic source and sensor distributions in Fourier collocation methods. The Journal of the Acoustical Society of America, 146(1), 278–288.](https://doi.org/10.1121/1.5116132) ([PDF](http://bug.medphys.ucl.ac.uk/papers/2019-Wise-JASA.pdf))docs/conf.py (1)
37-49: Optional: include GITHUB_SHA as a stable fallback.
Improves link stability on CI when branch/env vars are missing.def _detect_current_branch() -> str: @@ - return ( + return ( os.getenv("_GITHUB_BRANCH") or os.getenv("READTHEDOCS_GIT_IDENTIFIER") or os.getenv("READTHEDOCS_GIT_BRANCH") or os.getenv("GITHUB_REF_NAME") + or os.getenv("GITHUB_SHA") # final stable fallback or "master" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
.github/workflows/link-check.yml(1 hunks)docs/README.md(2 hunks)docs/conf.py(2 hunks)docs/examples_guide.rst(1 hunks)docs/get_started/grid_overview.rst(1 hunks)docs/get_started/medium_overview.rst(1 hunks)docs/get_started/sensor_overview.rst(1 hunks)docs/get_started/source_overview.rst(1 hunks)docs/index.rst(1 hunks)docs/kwave.kWaveSimulation.rst(1 hunks)docs/kwave.kWaveSimulation_helper.rst(1 hunks)docs/kwave.kspaceFirstOrder3D.rst(1 hunks)docs/kwave.kspaceFirstOrderAS.rst(1 hunks)docs/kwave.ktransducer.rst(1 hunks)docs/kwave.options.rst(1 hunks)docs/kwave.recorder.rst(1 hunks)examples/README.md(1 hunks)examples/at_array_as_sensor/README.md(1 hunks)examples/at_array_as_source/README.md(1 hunks)examples/at_linear_array_transducer/README.md(1 hunks)examples/ivp_photoacoustic_waveforms/README.md(1 hunks)examples/na_controlling_the_pml/README.md(1 hunks)examples/pr_2D_FFT_line_sensor/README.md(1 hunks)examples/pr_2D_TR_line_sensor/README.md(1 hunks)examples/pr_3D_FFT_planar_sensor/README.md(1 hunks)examples/pr_3D_TR_planar_sensor/README.md(1 hunks)examples/sd_directivity_modelling_2D/README.md(1 hunks)examples/sd_focussed_detector_2D/README.md(1 hunks)examples/sd_focussed_detector_3D/README.md(1 hunks)examples/us_beam_patterns/README.md(1 hunks)examples/us_bmode_linear_transducer/README.md(1 hunks)examples/us_bmode_phased_array/README.md(1 hunks)examples/us_defining_transducer/README.md(1 hunks)kwave/options/simulation_options.py(1 hunks)kwave/utils/angular_spectrum.py(1 hunks)kwave/utils/angular_spectrum_cw.py(1 hunks)tests/test_us_beam_patterns.py(1 hunks)
✅ Files skipped from review due to trivial changes (15)
- examples/sd_directivity_modelling_2D/README.md
- examples/pr_3D_TR_planar_sensor/README.md
- examples/sd_focussed_detector_2D/README.md
- examples/us_beam_patterns/README.md
- examples/pr_3D_FFT_planar_sensor/README.md
- examples/us_defining_transducer/README.md
- examples/us_bmode_linear_transducer/README.md
- examples/us_bmode_phased_array/README.md
- examples/at_array_as_sensor/README.md
- examples/pr_2D_FFT_line_sensor/README.md
- examples/na_controlling_the_pml/README.md
- examples/pr_2D_TR_line_sensor/README.md
- tests/test_us_beam_patterns.py
- kwave/options/simulation_options.py
- docs/kwave.kWaveSimulation_helper.rst
🚧 Files skipped from review as they are similar to previous changes (12)
- docs/get_started/medium_overview.rst
- docs/get_started/sensor_overview.rst
- .github/workflows/link-check.yml
- docs/kwave.kspaceFirstOrderAS.rst
- docs/kwave.ktransducer.rst
- docs/README.md
- docs/get_started/grid_overview.rst
- docs/get_started/source_overview.rst
- docs/kwave.options.rst
- docs/examples_guide.rst
- docs/kwave.kspaceFirstOrder3D.rst
- docs/index.rst
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-11T16:12:04.842Z
Learnt from: waltsims
PR: waltsims/k-wave-python#625
File: docs/get_started/first_simulation.rst:147-154
Timestamp: 2025-09-11T16:12:04.842Z
Learning: In k-Wave Python, kspaceFirstOrder2D function takes separate simulation_options and execution_options parameters, not a single options dict. Use SimulationOptions and SimulationExecutionOptions classes from kwave.options, with snake_case attributes like pml_inside instead of MATLAB-style PMLInside.
Applied to files:
docs/kwave.kWaveSimulation.rst
🪛 markdownlint-cli2 (0.17.2)
examples/at_array_as_source/README.md
7-7: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (10)
examples/sd_focussed_detector_3D/README.md (2)
3-3: Colab badge updated to HEAD — looks good.Using
blob/HEADkeeps links resilient to default-branch renames and matches the repo-wide convention.
3-3: Notebook present — Colab link safeConfirmed examples/sd_focussed_detector_3D/sd_focussed_detector_3D.ipynb exists on the PR branch (script output: "Notebook found ✅"). Colab badge should not 404.
examples/at_linear_array_transducer/README.md (1)
3-3: Link update improves maintainability by using HEAD.This change updates the Colab link to use
HEADinstead of a specific commit hash, which ensures users always access the latest version of the notebook. This is consistent with similar updates across other example READMEs in this PR.kwave/utils/angular_spectrum_cw.py (1)
83-84: GPU data_cast options correctly removed.The validation now correctly restricts
data_castto only["off", "double", "single"], removing the GPU options that are not supported in the Python implementation. This prevents users from passing invalid GPU-related values that would later trigger the NotImplementedError.kwave/utils/angular_spectrum.py (1)
96-97: GPU data_cast options correctly removed.The validation now correctly restricts
data_castto only["off", "double", "single"], removing the GPU options that are not supported in the Python implementation. This is consistent with the similar change inangular_spectrum_cw.py.examples/ivp_photoacoustic_waveforms/README.md (1)
3-3: Colab link switched to HEAD — good call.
Avoids default-branch drift and keeps badges working across forks.docs/kwave.recorder.rst (1)
1-7: Autodoc options expansion looks good.
:undoc-members: and :show-inheritance: improve API surface visibility.examples/at_array_as_source/README.md (1)
3-3: Colab badge updated to HEAD — LGTM.
Consistent with the repo-wide shift.docs/kwave.kWaveSimulation.rst (1)
1-9: Good doc polish.
Anchor + expanded autodoc options align with the new docs style.examples/README.md (1)
9-10: HEAD-based Colab links across examples — nice consistency.
Reduces default-branch coupling and broken-badge churn.Also applies to: 12-12, 13-13, 15-15, 16-18, 20-21, 23-27, 29-33, 36-36, 38-38, 40-40
Summary by CodeRabbit
New Features
Documentation
Chores
Bug Fixes