Fullwave example #650
Fullwave example #650
Conversation
This script replicates the fullwave25 simple_plane_wave example using k-wave, including methods for mapping coordinates and generating a Gaussian-modulated sinusoidal signal. It sets up the simulation parameters, defines the acoustic medium properties, initializes the pressure source, runs the simulation, and visualizes the results.
Added README for Simple Plane Wave example showcasing compatibility with fullwave25.
Removed saving of animation to MP4 and closing of plots.
WalkthroughAdds a new "Simple Plane Wave" example: a README, a Jupyter notebook, and a Python script. Introduces two utility functions (coordinate mapping and Gaussian‑modulated sinusoid) and provides a complete 2D k‑Wave simulation workflow with layered source timing, sensor capture, and visualizations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Setup as Simulation Setup
participant Signal as Signal Generator
participant Engine as k‑Wave Engine
participant Output as Visualization
User->>Setup: define domain, grid, medium
User->>Signal: call gaussian_modulated_sinusoidal_signal(...)
Signal->>Signal: compute Gaussian‑modulated sinusoid (apply optional layer delay)
Signal-->>User: return time-series pulse
User->>Setup: call map_to_coords(source_mask)
Setup-->>User: return source coordinates
User->>Engine: configure Source, Sensor, options (GPU, disk IO)
Engine->>Engine: execute 2D simulation
Engine-->>Output: sensor data
Output->>Output: reshape, snapshot, animate, save
Output-->>User: visualization files / frames
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #650 +/- ##
=======================================
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:
|
![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
🧹 Nitpick comments (4)
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.py (2)
57-123: Pulse generator logic looks good; consider stricterNonechecksThe Gaussian-modulated sinusoid with optional layer delay is implemented consistently with the docstring and the way it’s used in this example (layer indices 0..
element_thickness_px-1), so the behavior is correct here.For robustness and readability, you might consider making the guard explicit about
Nonerather than relying on truthiness:- if i_layer: - assert dt_for_layer_delay, "dt must be provided if i_layer is provided" - assert cfl_for_layer_delay, "cfl must be provided if i_layer is provided" + if i_layer is not None: + assert dt_for_layer_delay is not None, "dt must be provided if i_layer is provided" + assert cfl_for_layer_delay is not None, "cfl must be provided if i_layer is provided" t = t - (dt_for_layer_delay / cfl_for_layer_delay) * i_layerThis avoids edge cases where
i_layer == 0or very smalldt_for_layer_delay/cfl_for_layer_delaycould accidentally bypass the asserts.
261-305: Guard againstskip_every_n_frame == 0in animation loopFor the current parameter set,
grid.Ntis large enough thatskip_every_n_frame = int(grid.Nt / num_plot_image)is safely ≥ 1. For future reuse with smallerNt, this could become 0 and break the slice step.A small defensive tweak keeps the example robust:
-num_plot_image: int = 50 -skip_every_n_frame =int(grid.Nt / num_plot_image) +num_plot_image: int = 50 +skip_every_n_frame = max(1, int(grid.Nt / num_plot_image))This prevents a zero step in
propagation_map[::skip_every_n_frame, ...].examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.ipynb (2)
65-170: Helper functions mirror the script; keep them in sync going forwardThe notebook versions of
map_to_coordsandgaussian_modulated_sinusoidal_signalare consistent with the script implementation and safe (nonp.boolalias here). This is good for users who only run the notebook.Given these are effectively duplicated definitions, consider treating the script as the single source of truth and importing from it in the notebook, or at least keeping them strictly in sync whenever behavior changes, so the example doesn’t diverge over time.
393-445: Optional: protect against zero frame step in animationAs in the script,
skip_every_n_frame = int(grid.Nt / num_plot_image)is fine for this particular setup but could become 0 if reused with smallNt, which would break the slice step.You can harden this with:
-num_plot_image: int = 50 -skip_every_n_frame = int(grid.Nt / num_plot_image) +num_plot_image: int = 50 +skip_every_n_frame = max(1, int(grid.Nt / num_plot_image))This keeps the animation loop valid for any reasonable
Nt.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/fullwave_simple_plane_wave/README.md(1 hunks)examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.ipynb(1 hunks)examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: waltsims
Repo: waltsims/k-wave-python PR: 625
File: docs/get_started/first_simulation.rst:147-154
Timestamp: 2025-09-11T16:12:04.876Z
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.
🧬 Code graph analysis (1)
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.py (9)
kwave/data.py (1)
Vector(7-49)kwave/kgrid.py (6)
kWaveGrid(14-701)dx(178-182)dy(185-189)Nx(157-161)Ny(164-168)size(297-301)kwave/kmedium.py (1)
kWaveMedium(11-232)kwave/ksensor.py (1)
kSensor(9-81)kwave/ksource.py (1)
kSource(11-382)kwave/utils/colormap.py (1)
get_color_map(10-38)kwave/kspaceFirstOrder2D.py (1)
kspaceFirstOrder2D(141-363)kwave/options/simulation_execution_options.py (1)
SimulationExecutionOptions(13-308)kwave/options/simulation_options.py (1)
SimulationOptions(46-348)
🪛 GitHub Actions: Ruff
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.ipynb
[error] 1-1: Ruff: Import block is un-sorted or un-formatted (I001).
🪛 GitHub Check: ruff
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.ipynb
[failure] 1-1: Ruff (I001)
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.ipynb:1:1: I001 Import block is un-sorted or un-formatted
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.py
[failure] 6-25: Ruff (I001)
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.py:6:1: I001 Import block is un-sorted or un-formatted
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: collect_references
🔇 Additional comments (4)
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.py (2)
170-175: Medium setup matches Fullwave25 objective (alpha_power=1.1)The medium definition correctly embeds a central object with modified
sound_speed,density, andalpha_coeff, and crucially setsalpha_power=1.1, satisfying the requested attenuation exponent change for comparison with Fullwave25.The use of
kWaveMediumhere aligns with the library’s patterns.
220-229: The review comment is based on an incorrect assumption about how the library operates.After thorough verification of the k-wave-python codebase:
kwave_function_nameis never used for runtime execution decisions. While it defaults to"kspaceFirstOrder3D"inSimulationExecutionOptions, it exists only as a stored parameter and test assertion. There are no references to it elsewhere in the codebase.Binary selection is based solely on the
is_gpu_simulationflag. Thebinary_nameproperty inSimulationExecutionOptionsselects the binary based on whetheris_gpu_simulationis True or False—not onkwave_function_name:
is_gpu_simulation=True→ uses"kspaceFirstOrder-CUDA"is_gpu_simulation=False→ uses"kspaceFirstOrder-OMP"The same GPU binary handles both 2D and 3D simulations. According to the docstring in
kspace_first_order_2d_gpu: "The GPU version uses the same binary for both 2D and 3D simulations." Thekspace_first_order_2d_gpuwrapper setsis_gpu_simulation=Trueand then delegates tokspaceFirstOrder2D, which works correctly.The example code is correct as written. The provided code snippet at lines 220–229 requires no changes. Explicitly setting
kwave_function_name="kspaceFirstOrder2D"would have no effect on execution.Likely an incorrect or invalid review comment.
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.ipynb (2)
220-246: Medium in notebook also usesalpha_power=1.1as requiredThe notebook’s medium definition matches the script and sets
alpha_power=1.1, so it meets the Fullwave25 comparison requirement as well as the standalone script.This consistency between the two implementations is good.
326-343: Remove the suggestedkwave_function_nameparameter—it is unused dead code in the library and has no effect on 2D GPU simulations.The
kwave_function_nameparameter inSimulationExecutionOptionsis defined but never read anywhere in the codebase. The actual function selection for 2D vs. 3D is determined by which simulation function is called (e.g.,kspaceFirstOrder2D()) and the grid dimensions implicit in the grid object, not by this parameter. The code in the review snippet is already correct as-is; settingkwave_function_namewould have no practical effect.Likely an incorrect or invalid review comment.
| "cell_type": "code", | ||
| "execution_count": null, | ||
| "id": "36d103c4-d931-4a53-aa5e-4a70a794b95e", | ||
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "from copy import deepcopy\n", | ||
| "\n", | ||
| "from IPython.display import HTML\n", | ||
| "from matplotlib import animation\n", | ||
| "import matplotlib.pyplot as plt\n", | ||
| "import numpy as np\n", | ||
| "\n", | ||
| "from mpl_toolkits.axes_grid1 import make_axes_locatable\n", | ||
| "from numpy.typing import NDArray\n", | ||
| "from tqdm import tqdm\n", | ||
| "\n", | ||
| "from kwave.data import Vector\n", | ||
| "from kwave.kgrid import kWaveGrid\n", | ||
| "from kwave.kmedium import kWaveMedium\n", | ||
| "from kwave.ksensor import kSensor\n", | ||
| "from kwave.ksource import kSource\n", | ||
| "from kwave.utils.colormap import get_color_map\n", | ||
| "from kwave.kspaceFirstOrder2D import kspaceFirstOrder2D\n", | ||
| "\n", | ||
| "from kwave.options.simulation_execution_options import SimulationExecutionOptions\n", | ||
| "from kwave.options.simulation_options import SimulationOptions" | ||
| ] |
There was a problem hiding this comment.
Sort imports in the notebook’s import cell to satisfy Ruff I001
Ruff is flagging the notebook (via its exported .py) for unsorted imports. To make the lint pass, reorder the imports in this cell into logical groups and alphabetical order, similar to:
-from copy import deepcopy
-
-from IPython.display import HTML
-from matplotlib import animation
-import matplotlib.pyplot as plt
-import numpy as np
-
-from mpl_toolkits.axes_grid1 import make_axes_locatable
-from numpy.typing import NDArray
-from tqdm import tqdm
-
-from kwave.data import Vector
-from kwave.kgrid import kWaveGrid
-from kwave.kmedium import kWaveMedium
-from kwave.ksensor import kSensor
-from kwave.ksource import kSource
-from kwave.utils.colormap import get_color_map
-from kwave.kspaceFirstOrder2D import kspaceFirstOrder2D
-
-from kwave.options.simulation_execution_options import SimulationExecutionOptions
-from kwave.options.simulation_options import SimulationOptions
+from copy import deepcopy
+
+from IPython.display import HTML
+import matplotlib.pyplot as plt
+import numpy as np
+from matplotlib import animation
+from mpl_toolkits.axes_grid1 import make_axes_locatable
+from numpy.typing import NDArray
+from tqdm import tqdm
+
+from kwave.data import Vector
+from kwave.kgrid import kWaveGrid
+from kwave.kmedium import kWaveMedium
+from kwave.ksensor import kSensor
+from kwave.ksource import kSource
+from kwave.kspaceFirstOrder2D import kspaceFirstOrder2D
+from kwave.options.simulation_execution_options import SimulationExecutionOptions
+from kwave.options.simulation_options import SimulationOptions
+from kwave.utils.colormap import get_color_mapAdjust as needed to align with your configured import-sorting rules.
🤖 Prompt for AI Agents
In examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.ipynb around
lines 27 to 54, the imports in the first code cell are not sorted and trigger
Ruff I001; reorder them into logical groups (standard library, third‑party,
local/package) and alphabetically within each group (e.g., copy, typing/NDArray,
IPython.display, matplotlib, mpl_toolkits, numpy, tqdm, then kwave submodules
grouped together and alphabetized) so the exported .py matches your
import-sorting rules and Ruff stops flagging I001.
| from copy import deepcopy | ||
|
|
||
| from matplotlib import animation | ||
| import matplotlib.pyplot as plt | ||
| import numpy as np | ||
|
|
||
| from mpl_toolkits.axes_grid1 import make_axes_locatable | ||
| from numpy.typing import NDArray | ||
| from tqdm import tqdm | ||
|
|
||
| from kwave.data import Vector | ||
| from kwave.kgrid import kWaveGrid | ||
| from kwave.kmedium import kWaveMedium | ||
| from kwave.ksensor import kSensor | ||
| from kwave.ksource import kSource | ||
| from kwave.utils.colormap import get_color_map | ||
| from kwave.kspaceFirstOrder2D import kspaceFirstOrder2D | ||
|
|
||
| from kwave.options.simulation_execution_options import SimulationExecutionOptions | ||
| from kwave.options.simulation_options import SimulationOptions |
There was a problem hiding this comment.
Fix Ruff I001 by sorting and grouping imports
Ruff is failing on this file due to unsorted/unformatted imports. Please group imports into standard library / third-party / local sections and sort within each group so the linter passes.
For example (adjust to your isort/Ruff config):
-from copy import deepcopy
-
-from matplotlib import animation
-import matplotlib.pyplot as plt
-import numpy as np
-
-from mpl_toolkits.axes_grid1 import make_axes_locatable
-from numpy.typing import NDArray
-from tqdm import tqdm
-
-from kwave.data import Vector
-from kwave.kgrid import kWaveGrid
-from kwave.kmedium import kWaveMedium
-from kwave.ksensor import kSensor
-from kwave.ksource import kSource
-from kwave.utils.colormap import get_color_map
-from kwave.kspaceFirstOrder2D import kspaceFirstOrder2D
-
-from kwave.options.simulation_execution_options import SimulationExecutionOptions
-from kwave.options.simulation_options import SimulationOptions
+from copy import deepcopy
+
+import matplotlib.pyplot as plt
+import numpy as np
+from matplotlib import animation
+from mpl_toolkits.axes_grid1 import make_axes_locatable
+from numpy.typing import NDArray
+from tqdm import tqdm
+
+from kwave.data import Vector
+from kwave.kgrid import kWaveGrid
+from kwave.kmedium import kWaveMedium
+from kwave.ksensor import kSensor
+from kwave.ksource import kSource
+from kwave.kspaceFirstOrder2D import kspaceFirstOrder2D
+from kwave.options.simulation_execution_options import SimulationExecutionOptions
+from kwave.options.simulation_options import SimulationOptions
+from kwave.utils.colormap import get_color_mapAdjust ordering as needed to match your configured style.
📝 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.
| from copy import deepcopy | |
| from matplotlib import animation | |
| import matplotlib.pyplot as plt | |
| import numpy as np | |
| from mpl_toolkits.axes_grid1 import make_axes_locatable | |
| from numpy.typing import NDArray | |
| from tqdm import tqdm | |
| from kwave.data import Vector | |
| from kwave.kgrid import kWaveGrid | |
| from kwave.kmedium import kWaveMedium | |
| from kwave.ksensor import kSensor | |
| from kwave.ksource import kSource | |
| from kwave.utils.colormap import get_color_map | |
| from kwave.kspaceFirstOrder2D import kspaceFirstOrder2D | |
| from kwave.options.simulation_execution_options import SimulationExecutionOptions | |
| from kwave.options.simulation_options import SimulationOptions | |
| from copy import deepcopy | |
| import matplotlib.pyplot as plt | |
| import numpy as np | |
| from matplotlib import animation | |
| from mpl_toolkits.axes_grid1 import make_axes_locatable | |
| from numpy.typing import NDArray | |
| from tqdm import tqdm | |
| from kwave.data import Vector | |
| from kwave.kgrid import kWaveGrid | |
| from kwave.kmedium import kWaveMedium | |
| from kwave.ksensor import kSensor | |
| from kwave.ksource import kSource | |
| from kwave.kspaceFirstOrder2D import kspaceFirstOrder2D | |
| from kwave.options.simulation_execution_options import SimulationExecutionOptions | |
| from kwave.options.simulation_options import SimulationOptions | |
| from kwave.utils.colormap import get_color_map |
🧰 Tools
🪛 GitHub Check: ruff
[failure] 6-25: Ruff (I001)
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.py:6:1: I001 Import block is un-sorted or un-formatted
🤖 Prompt for AI Agents
In examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.py around
lines 6 to 25, imports are not grouped/sorted which triggers Ruff I001; reorder
imports into three sections (standard library, third-party, local/package), sort
names alphabetically within each section (and within from-import lists), and
separate each section with a single blank line so the file matches isort/Ruff
expectations.
| # Simple Plane Wave | ||
|
|
||
| [](https://colab.research.google.com/github/waltsims/k-wave-python/blob/HEAD/examples/fullwave_simple_plane_wave.ipynb) | ||
|
|
||
| This example showcases the compatibility between [fullwave25](https://github.com/pinton-lab/fullwave25) and [k-wave-python](https://github.com/waltsims/k-wave-python). It directly copies two methods over. |
There was a problem hiding this comment.
Fix Colab badge link to point to the correct notebook path
The Colab badge currently links to:
.../examples/fullwave_simple_plane_wave.ipynb
but the notebook in this PR lives at:
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.ipynb
Recommend updating the URL accordingly:
-[](https://colab.research.google.com/github/waltsims/k-wave-python/blob/HEAD/examples/fullwave_simple_plane_wave.ipynb)
+[](https://colab.research.google.com/github/waltsims/k-wave-python/blob/HEAD/examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.ipynb)So users can open the example notebook directly from the README.
🤖 Prompt for AI Agents
In examples/fullwave_simple_plane_wave/README.md around lines 1 to 5, the Colab
badge URL points to examples/fullwave_simple_plane_wave.ipynb but the notebook
actually resides at
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.ipynb; update the
badge link to the correct path (replace the URL portion after
/github/.../blob/HEAD/ with
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.ipynb) so the
badge opens the notebook directly in Colab.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.py (2)
6-23: Fix import ordering to resolve Ruff I001 failure.The import block remains unsorted/ungrouped, causing the static analysis check to fail. This was previously flagged and needs to be addressed.
28-28: Update deprecatednp.booltype hint.The use of
np.boolin the type annotation is deprecated in NumPy 2.x and will cause import failures. This was previously flagged and needs to be addressed.
🧹 Nitpick comments (6)
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.py (6)
39-44: Remove commented-out code.Lines 39, 42, and 43 contain commented-out code that should be removed to improve code clarity.
Apply this diff:
def map_to_coords( map_data: NDArray[np.float64 | np.int64 | np.bool], *, export_as_xyz: bool = False, ) -> NDArray[np.int64]: """Map the mask map to coordinates. Returns: NDArray[np.int64]: An array of coordinates corresponding to non-zero elements in the mask. """ is_3d = map_data.ndim == 3 - # indices = np.where(map_data.T != 0) indices = np.where(map_data != 0) if is_3d: - # out = np.array([indices[2], indices[1], indices[0]]).T - # out = np.array([indices[2], indices[1], indices[0]]).T out = np.array([*indices]).T if export_as_xyz: out = np.stack([out[:, 2], out[:, 1], out[:, 0]], axis=1)
130-146: Remove redundantdurationassignment.The variable
durationis assigned the same value on both line 130 and line 146. Remove the first assignment to avoid confusion.Apply this diff:
f0 = 3.0e6 c0 = 1540.0 -duration = domain_size[0] / c0 * 2 - cfl = 0.2 ppw = 12
186-200: Consider renamingn_yfor clarity.The variable
n_yon line 199 represents the number of source points per layer, not specifically a y-dimension count. Consider renaming it topoints_per_layerfor better code readability.# assign the source signal to the corresponding layer - n_y = p_coordinates.shape[0] // element_thickness_px - p0[n_y * i_thickness : n_y * (i_thickness + 1), :] = p0_vec.copy() + points_per_layer = p_coordinates.shape[0] // element_thickness_px + p0[points_per_layer * i_thickness : points_per_layer * (i_thickness + 1), :] = p0_vec.copy()
262-262: Use floor division for integer calculation.Line 262 uses regular division followed by
int()conversion. For clarity and intent, use floor division//directly.-skip_every_n_frame =int(grid.Nt / num_plot_image) +skip_every_n_frame = grid.Nt // num_plot_image
276-276: Remove commented-out code.Line 276 contains a commented-out transpose operation that should be removed for code clarity.
animation_list = [] -# propagation_map = propagation_map.transpose(2, 0, 1) for i, p_map_i in tqdm(
291-294: Consider increasing font size for better readability.The timestamp text uses
fontsize=4, which may be too small to read clearly in the saved animation. Consider increasing to at least 8-10 for better visibility.# set text to show the current time step - text = ax.text(0.5, 1.05, f"t = {i * skip_every_n_frame} / {propagation_map.shape[0]}", fontsize=4, + text = ax.text(0.5, 1.05, f"t = {i * skip_every_n_frame} / {propagation_map.shape[0]}", fontsize=10, ha="center", animated=True, transform=ax.transAxes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.py (3)
kwave/kgrid.py (1)
setTime(134-154)kwave/ksource.py (1)
kSource(11-382)kwave/utils/colormap.py (1)
get_color_map(10-38)
🪛 GitHub Check: ruff
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.py
[failure] 6-23: Ruff (I001)
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.py:6:1: I001 Import block is un-sorted or un-formatted
⏰ 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). (12)
- GitHub Check: test (macos-latest, 3.12)
- GitHub Check: test (macos-latest, 3.13)
- GitHub Check: test (windows-latest, 3.12)
- GitHub Check: test (macos-latest, 3.11)
- GitHub Check: test (windows-latest, 3.13)
- GitHub Check: test (ubuntu-latest, 3.13)
- GitHub Check: test (ubuntu-latest, 3.10)
- GitHub Check: test (macos-latest, 3.10)
- GitHub Check: test (ubuntu-latest, 3.12)
- GitHub Check: test (windows-latest, 3.10)
- GitHub Check: test (ubuntu-latest, 3.11)
- GitHub Check: test (windows-latest, 3.11)
🔇 Additional comments (1)
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.py (1)
169-172: LGTM - Attenuation exponent correctly set to 1.1.The medium configuration correctly sets
alpha_power=1.1, matching the PR objective to modify the Fullwave25 example with this specific attenuation exponent.
| if i_layer: | ||
| assert dt_for_layer_delay, "dt must be provided if i_layer is provided" | ||
| assert cfl_for_layer_delay, "cfl must be provided if i_layer is provided" | ||
| t = t - (dt_for_layer_delay / cfl_for_layer_delay) * i_layer |
There was a problem hiding this comment.
Fix truthiness check that incorrectly handles i_layer=0.
Line 107 uses if i_layer: which evaluates to False when i_layer=0, causing the layer delay calculation to be skipped for the first layer (index 0). Similarly, the assertions on lines 108-109 would incorrectly pass when i_layer=0 even if dt_for_layer_delay or cfl_for_layer_delay are None.
Apply this diff:
- if i_layer:
- assert dt_for_layer_delay, "dt must be provided if i_layer is provided"
- assert cfl_for_layer_delay, "cfl must be provided if i_layer is provided"
+ if i_layer is not None:
+ assert dt_for_layer_delay is not None, "dt must be provided if i_layer is provided"
+ assert cfl_for_layer_delay is not None, "cfl must be provided if i_layer is provided"
t = t - (dt_for_layer_delay / cfl_for_layer_delay) * i_layer📝 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.
| if i_layer: | |
| assert dt_for_layer_delay, "dt must be provided if i_layer is provided" | |
| assert cfl_for_layer_delay, "cfl must be provided if i_layer is provided" | |
| t = t - (dt_for_layer_delay / cfl_for_layer_delay) * i_layer | |
| if i_layer is not None: | |
| assert dt_for_layer_delay is not None, "dt must be provided if i_layer is provided" | |
| assert cfl_for_layer_delay is not None, "cfl must be provided if i_layer is provided" | |
| t = t - (dt_for_layer_delay / cfl_for_layer_delay) * i_layer |
🤖 Prompt for AI Agents
In examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.py around
lines 107 to 110, the conditional and assertions use truthiness checks that
treat i_layer==0 as False and won't validate provided
dt_for_layer_delay/cfl_for_layer_delay; change the guard to check for presence
explicitly (e.g., if i_layer is not None:) and replace the assertions with
explicit None checks (e.g., assert dt_for_layer_delay is not None, assert
cfl_for_layer_delay is not None) so the layer delay calculation runs for
i_layer==0 and missing parameters are caught.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.py (1)
104-111: Use explicitNonechecks fori_layerand layer-delay parametersThe guard
if i_layer:treatsi_layer=0as false, so the layer-delay logic and assertions are skipped for the first layer even wheni_layeris intentionally provided. It also won’t catch missingdt_for_layer_delay/cfl_for_layer_delaywheni_layer=0.Switch to explicit
Nonechecks:- if i_layer: - assert dt_for_layer_delay, "dt must be provided if i_layer is provided" - assert cfl_for_layer_delay, "cfl must be provided if i_layer is provided" + if i_layer is not None: + assert dt_for_layer_delay is not None, "dt must be provided if i_layer is provided" + assert cfl_for_layer_delay is not None, "cfl must be provided if i_layer is provided" t = t - (dt_for_layer_delay / cfl_for_layer_delay) * i_layerThis keeps behavior identical for positive
i_layerwhile correctly handlingi_layer=0and validating required parameters.
🧹 Nitpick comments (2)
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.py (2)
1-4: Fix minor docstring typos and function name referenceTop-level docstring and the pulse docstring have small wording issues (
maps-to_coordsvsmap_to_coords, repeatedemmitted). Cleaning these up will better match the actual API and improve readability.-""" -k-wave python example which replicates the fullwave25 simple_plane_wave example, directly -copying methods (maps-to_coords and gaussian_modulated_sinusoidal_signal) from the package. -""" +""" +k-wave Python example which replicates the fullwave25 simple_plane_wave example, +copying methods (map_to_coords and gaussian_modulated_sinusoidal_signal) from the package. +""" @@ - This variable is used to shift the pulse signal in time - so that the signal is emmitted within the transducer layer correctly. + This variable is used to shift the pulse signal in time + so that the signal is emitted within the transducer layer correctly. @@ - This variable is used to shift the pulse signal in time - so that the signal is emmitted within the transducer layer correctly. + This variable is used to shift the pulse signal in time + so that the signal is emitted within the transducer layer correctly. @@ - This variable is used to shift the pulse signal in time - so that the signal is emmitted within the transducer layer correctly. + This variable is used to shift the pulse signal in time + so that the signal is emitted within the transducer layer correctly.Also applies to: 83-96
27-52: Remove stale commented-out alternatives inmap_to_coordsThe multiple commented-out variants of the indexing logic make the helper harder to read without adding value now that the final behavior is decided.
You can simplify by dropping the dead code:
- # indices = np.where(map_data.T != 0) - indices = np.where(map_data != 0) + indices = np.where(map_data != 0) @@ - # out = np.array([indices[2], indices[1], indices[0]]).T - # out = np.array([indices[2], indices[1], indices[0]]).T out = np.array([*indices]).T
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.py(1 hunks)
🧰 Additional context used
🪛 GitHub Check: ruff
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.py
[failure] 6-23: Ruff (I001)
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.py:6:1: I001 Import block is un-sorted or un-formatted
⏰ 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). (12)
- GitHub Check: test (macos-latest, 3.13)
- GitHub Check: test (macos-latest, 3.12)
- GitHub Check: test (ubuntu-latest, 3.12)
- GitHub Check: test (macos-latest, 3.11)
- GitHub Check: test (windows-latest, 3.13)
- GitHub Check: test (ubuntu-latest, 3.13)
- GitHub Check: test (ubuntu-latest, 3.11)
- GitHub Check: test (ubuntu-latest, 3.10)
- GitHub Check: test (windows-latest, 3.10)
- GitHub Check: test (macos-latest, 3.10)
- GitHub Check: test (windows-latest, 3.12)
- GitHub Check: test (windows-latest, 3.11)
🔇 Additional comments (1)
examples/fullwave_simple_plane_wave/fullwave_simple_plane_wave.py (1)
168-172: Medium setup andalpha_power=1.1match the Fullwave25 example requirementsThe medium construction with layered
sound_speed,density, andalpha_coeff, plusalpha_power=1.1, aligns with the linked Fullwave25 simple_plane_wave example and the issue requirement to use attenuation exponent 1.1.No changes needed here.
| fig, ax = plt.subplots(1, 1, figsize=(4,6)) | ||
|
|
||
| num_plot_image: int = 50 | ||
| skip_every_n_frame =int(grid.Nt / num_plot_image) | ||
|
|
||
| c_map = medium.sound_speed | ||
| rho_map = medium.density | ||
|
|
||
| start = 0 | ||
| end = None | ||
| z_map = c_map * rho_map | ||
| z_map = (z_map - np.min(z_map)) / (np.max(z_map) - np.min(z_map) + 1e-9) | ||
|
|
||
| z_map_offset = p_max_plot * 0.8 | ||
|
|
||
| animation_list = [] | ||
|
|
||
| # propagation_map = propagation_map.transpose(2, 0, 1) | ||
| for i, p_map_i in tqdm( | ||
| enumerate(propagation_map[::skip_every_n_frame, start:end, start:end]), | ||
| total=len(propagation_map[::skip_every_n_frame, start:end, start:end]), | ||
| desc="plotting animation"): | ||
|
|
||
| processed_p_map = p_map_i + z_map_offset * (z_map) | ||
|
|
||
| image2 = ax.imshow( | ||
| processed_p_map, | ||
| vmin=-p_max_plot, | ||
| vmax=p_max_plot, | ||
| interpolation="nearest" | ||
| ) | ||
| # set text to show the current time step | ||
| text = ax.text(0.5, 1.05, f"t = {i * skip_every_n_frame} / {propagation_map.shape[0]}", fontsize=4, | ||
| ha="center", | ||
| animated=True, | ||
| transform=ax.transAxes) | ||
| animation_list.append([image2, text]) | ||
|
|
||
| animation_data = animation.ArtistAnimation( | ||
| fig, | ||
| animation_list, | ||
| interval=150, | ||
| blit=True, | ||
| repeat_delay=500, | ||
| ) |
There was a problem hiding this comment.
Guard animation frame step against zero and reuse sliced frames
If grid.Nt is ever smaller than num_plot_image, skip_every_n_frame becomes 0, which will cause a slicing error (slice step cannot be zero). Also, the same slice of propagation_map is computed twice.
You can make this more robust and slightly more efficient:
-num_plot_image: int = 50
-skip_every_n_frame =int(grid.Nt / num_plot_image)
+num_plot_image: int = 50
+skip_every_n_frame = max(1, grid.Nt // num_plot_image)
@@
-animation_list = []
-
-# propagation_map = propagation_map.transpose(2, 0, 1)
-for i, p_map_i in tqdm(
- enumerate(propagation_map[::skip_every_n_frame, start:end, start:end]),
- total=len(propagation_map[::skip_every_n_frame, start:end, start:end]),
+animation_list = []
+
+frames = propagation_map[::skip_every_n_frame, start:end, start:end]
+for i, p_map_i in tqdm(
+ enumerate(frames),
+ total=len(frames),1 participant
Fixes #649.
Summary by CodeRabbit
Documentation
New Features
✏️ Tip: You can customize this high-level summary in your review settings.