Consolidate cartesian interp option and introduce structured SimulationResult return #647
Conversation
WalkthroughAdds a new public data type Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Executor
participant NativeSim as "Native Simulation (C/engine)"
participant SimResult as "SimulationResult.from_dotdict"
Caller->>Executor: run_simulation(input, output, options)
Executor->>NativeSim: invoke simulation with files/options
NativeSim-->>Executor: sensor_data (dict)
Note right of Executor: previously returned raw dict/dotdict
Executor->>SimResult: SimulationResult.from_dotdict(sensor_data)
SimResult-->>Executor: SimulationResult instance
Executor-->>Caller: SimulationResult
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
kwave/data.py(2 hunks)kwave/executor.py(3 hunks)kwave/kspaceFirstOrder2D.py(3 hunks)kwave/kspaceFirstOrder3D.py(4 hunks)kwave/kspaceFirstOrderAS.py(5 hunks)kwave/options/simulation_options.py(0 hunks)tests/test_executor.py(3 hunks)tests/test_ivp_3D_simulation.py(2 hunks)tests/test_tvsp_3D_simulation.py(2 hunks)
💤 Files with no reviewable changes (1)
- kwave/options/simulation_options.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-11T16:12:04.876Z
Learnt from: waltsims
PR: waltsims/k-wave-python#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.
Applied to files:
tests/test_ivp_3D_simulation.pytests/test_tvsp_3D_simulation.pykwave/kspaceFirstOrder2D.pykwave/executor.pykwave/kspaceFirstOrder3D.pykwave/kspaceFirstOrderAS.py
🧬 Code graph analysis (7)
tests/test_ivp_3D_simulation.py (1)
kwave/kWaveSimulation.py (1)
kWaveSimulation(34-1552)
kwave/data.py (1)
kwave/kgrid.py (3)
Nx(157-161)Ny(164-168)Nz(171-175)
kwave/kspaceFirstOrder2D.py (1)
kwave/data.py (1)
SimulationResult(125-236)
kwave/executor.py (1)
kwave/data.py (2)
SimulationResult(125-236)from_dotdict(196-236)
kwave/kspaceFirstOrder3D.py (1)
kwave/data.py (1)
SimulationResult(125-236)
tests/test_executor.py (1)
kwave/data.py (1)
SimulationResult(125-236)
kwave/kspaceFirstOrderAS.py (1)
kwave/data.py (1)
SimulationResult(125-236)
🪛 GitHub Actions: pytest
tests/test_executor.py
[error] 221-221: TestExecutor.test_sensor_data_cropping_with_pml_inside: AssertionError - expected sensor_data['p_final'] to match mock values; actual value did not equal expected (1 != 20).
[error] 192-192: TestExecutor.test_sensor_data_cropping_with_pml_outside: getitem on p_final was not called as expected; cropping did not occur.
🔇 Additional comments (9)
tests/test_tvsp_3D_simulation.py (1)
75-75: LGTM! Parameter renamed to use the consolidated API.The change from
cart_interptocartesian_interpcorrectly reflects the API consolidation mentioned in the PR objectives.kwave/executor.py (1)
9-9: LGTM! Clean integration of SimulationResult return type.The changes correctly:
- Import the new SimulationResult type
- Annotate the return type (non-Optional is correct since there are no early returns in this method)
- Wrap the dotdict in SimulationResult after PML cropping
The implementation preserves the existing cropping logic while providing a structured return type with backward compatibility.
Also applies to: 36-36, 65-65
tests/test_ivp_3D_simulation.py (1)
73-73: LGTM! Parameter renamed to match consolidated API.Consistent with the broader API cleanup replacing
cart_interpwithcartesian_interp.kwave/data.py (1)
124-236: LGTM! Well-structured dataclass with good backward compatibility.The SimulationResult implementation is clean and well-designed:
- Clear separation between required grid metadata and optional sensor data fields
- Backward-compatible dict-like access via
__getitem__and__contains__- Safe construction from dotdict with sensible defaults
- Appropriate type hints throughout
The
from_dotdictclassmethod properly handles missing keys and type conversions, ensuring robust construction from HDF5-loaded data.kwave/kspaceFirstOrderAS.py (3)
2-2: LGTM! Return types correctly annotated as Optional[SimulationResult].The Optional return type is appropriate because of the early return at line 366 when
save_to_disk_exit=True. The imports are correctly added to support the new type annotations.Also applies to: 7-7, 34-34, 93-93
162-162: Improved warning message clarity.The warning message appears to have been improved (possibly fixing a typo or improving wording). Without seeing the old version, this looks like a reasonable refinement.
300-300: Assertion message consolidated to single line.Good cleanup - keeping assertion messages as single strings improves readability and maintainability.
kwave/kspaceFirstOrder2D.py (1)
1-1: LGTM! Return type annotations correctly added.The Optional[SimulationResult] return type is appropriate for both functions due to the early return path at line 359 when
save_to_disk_exit=True. This provides better type safety and IDE support.Also applies to: 6-6, 89-89, 149-149
kwave/kspaceFirstOrder3D.py (1)
1-1: LGTM! Return type annotations consistently applied across all 3D simulation functions.The Optional[SimulationResult] return type is correctly applied to all three functions (GPU, CPU wrapper, and main function) to account for the early return at line 371 when
save_to_disk_exit=True.Also applies to: 5-5, 29-29, 83-83, 140-140
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #647 +/- ##
==========================================
+ Coverage 73.95% 74.05% +0.09%
==========================================
Files 50 50
Lines 7000 7038 +38
Branches 1338 1339 +1
==========================================
+ Hits 5177 5212 +35
- Misses 1280 1282 +2
- Partials 543 544 +1
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:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_executor.py (2)
177-209: Cropping test correctly updated for SimulationResult.The test logic is sound:
- Tracking mocks verify that
_crop_pmlis called with correct slice operations- Field assertions confirm non-cropped data is preserved
SimulationResultinstance check validates the return typeThe test effectively verifies the control flow and cropping behavior.
Optional: Consider strengthening verification of cropped values.
Currently, the test verifies that cropping functions were called but doesn't verify the final cropped array shapes or values in the
SimulationResult. You could enhance this by:# Configure mocks to return arrays with realistic shapes cropped_2d = np.zeros((16, 16)) # After cropping 20x20 with PML=2 cropped_3d = np.zeros((16, 16, 16)) original_two_d_output.__getitem__.return_value = cropped_2d original_three_d_output.__getitem__.return_value = cropped_3d # Then verify the shapes in the result self.assertEqual(sensor_data["p_final"].shape, (16, 16)) self.assertEqual(sensor_data["p_max_all"].shape, (16, 16, 16))This would provide stronger verification that the cropping logic produces the expected output, not just that it was invoked.
211-243: Correct verification that cropping is skipped when pml_inside is True.The test properly verifies that when
pml_inside=True, the cropping functions are not called and all fields remain unchanged in theSimulationResult.Optional: Same enhancement opportunity as the pml_outside test.
Consider configuring mocks with realistic array shapes and verifying the final array dimensions in the
SimulationResultto strengthen the test's verification of data correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_executor.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_executor.py (2)
kwave/data.py (1)
SimulationResult(125-236)kwave/executor.py (2)
Executor(14-144)run_simulation(36-65)
⏰ 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). (5)
- GitHub Check: test (macos-latest, 3.11)
- GitHub Check: test (macos-latest, 3.12)
- GitHub Check: test (macos-latest, 3.10)
- GitHub Check: test (macos-latest, 3.13)
- GitHub Check: test (macos-latest, 3.12)
🔇 Additional comments (3)
tests/test_executor.py (3)
12-12: LGTM: Import updated for SimulationResult.The import correctly reflects the updated return type from the executor.
66-67: Good fix: Using real dict for SimulationResult compatibility.This change is necessary because
SimulationResult.from_dotdictexpects a real dictionary with working.get()method semantics, which MagicMock doesn't provide correctly.
111-111: LGTM: Assertions correctly verify SimulationResult return type.The updated assertions properly check that
run_simulationreturnsSimulationResultinstances instead of dotdict.Also applies to: 131-131
|
|
||
|
|
||
| @dataclass | ||
| class SimulationResult: |
There was a problem hiding this comment.
consider using xarray as https://github.com/OpenwaterHealth/OpenLIFU-python/blob/main/src/openlifu/plan/solution.py
There was a problem hiding this comment.
I think to increase utility of this, you need more data: specifically the spatial and temporal resolution.
For 2D simulations is Nz (and dz) 1 or None?
If you want to take a data set and compute some key metrics you would need more information as well, such as the frequency (which is needed for the mechanical index). I think it should be an aim to have data from which all key metrics can be derived without needing input files.
| * cart_interp: Interpolation mode used to extract the pressure when a Cartesian sensor mask is given. | ||
| If set to 'nearest', duplicated data points are discarded and sensor_data | ||
| will be returned with fewer points than specified by sensor.mask (default = 'linear'). |
There was a problem hiding this comment.
Perhaps we document Cartesian interp with the old docstring?
I removed the duplicate
cart_interpoption and keptcartesian_interpto make the API clear and consistent, then switched the executor return from a loose dotdict to a typedSimulationResult. This cuts confusion around options, improves IDE help and readability, and makes it safer and simpler to work with outputs from the k‑Wave binaries.Changes
kwave/options/simulation_options.py: removed cart_interp, kept cartesian_interp, cleaned docs/validation.tests/test_tvsp_3D_simulation.py,tests/test_ivp_3D_simulation.py: switched to cartesian_interp.kwave/data.py: added SimulationResult dataclass (+ from_dotdict).kwave/executor.py: return SimulationResult from run_simulation.kwave/kspaceFirstOrder2D.py,kwave/kspaceFirstOrder3D.py,kwave/kspaceFirstOrderAS.py: updated return annotations/imports.tests/test_executor.py: assertions now expect SimulationResult.Compatibility
Tests
Summary by CodeRabbit
Breaking Changes
New Features
Tests