Consolidate cartesian interp option and introduce structured SimulationResult return#647
Consolidate cartesian interp option and introduce structured SimulationResult return#647faridyagubbayli wants to merge 6 commits into
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 75.06% 76.57% +1.50%
==========================================
Files 57 57
Lines 8126 8686 +560
Branches 1582 1804 +222
==========================================
+ Hits 6100 6651 +551
+ Misses 1405 1398 -7
- Partials 621 637 +16
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
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?
|
@greptile re-review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def __getitem__(self, key: str): | ||
| """ | ||
| Enable dictionary-style access for backward compatibility. | ||
|
|
||
| Args: | ||
| key: Field name to access | ||
|
|
||
| Returns: | ||
| Value of the field | ||
|
|
||
| Raises: | ||
| KeyError: If the field does not exist | ||
| """ | ||
| if hasattr(self, key): | ||
| return getattr(self, key) | ||
| raise KeyError(f"'{key}' field not found in SimulationResult") | ||
|
|
||
| def __contains__(self, key: str) -> bool: | ||
| """ | ||
| Enable dictionary-style membership testing for backward compatibility. | ||
|
|
||
| Args: | ||
| key: Field name to check | ||
|
|
||
| Returns: | ||
| True if the field exists, False otherwise | ||
| """ | ||
| return hasattr(self, key) |
There was a problem hiding this comment.
__contains__ and __getitem__ use hasattr, which returns True for any attribute on the object — including methods like from_dotdict, __init__, and dunder attributes. A caller doing 'from_dotdict' in result gets True and result['from_dotdict'] returns the classmethod rather than raising KeyError. The intent is backward compatibility for sensor data fields, so the check should be restricted to declared dataclass fields only.
| def __getitem__(self, key: str): | |
| """ | |
| Enable dictionary-style access for backward compatibility. | |
| Args: | |
| key: Field name to access | |
| Returns: | |
| Value of the field | |
| Raises: | |
| KeyError: If the field does not exist | |
| """ | |
| if hasattr(self, key): | |
| return getattr(self, key) | |
| raise KeyError(f"'{key}' field not found in SimulationResult") | |
| def __contains__(self, key: str) -> bool: | |
| """ | |
| Enable dictionary-style membership testing for backward compatibility. | |
| Args: | |
| key: Field name to check | |
| Returns: | |
| True if the field exists, False otherwise | |
| """ | |
| return hasattr(self, key) | |
| def __getitem__(self, key: str): | |
| """ | |
| Enable dictionary-style access for backward compatibility. | |
| Args: | |
| key: Field name to access | |
| Returns: | |
| Value of the field | |
| Raises: | |
| KeyError: If the field does not exist | |
| """ | |
| import dataclasses | |
| field_names = {f.name for f in dataclasses.fields(self)} | |
| if key in field_names: | |
| return getattr(self, key) | |
| raise KeyError(f"'{key}' field not found in SimulationResult") | |
| def __contains__(self, key: str) -> bool: | |
| """ | |
| Enable dictionary-style membership testing for backward compatibility. | |
| Args: | |
| key: Field name to check | |
| Returns: | |
| True if the field exists, False otherwise | |
| """ | |
| import dataclasses | |
| return key in {f.name for f in dataclasses.fields(self)} |
| @classmethod | ||
| def from_dotdict(cls, data: dict) -> "SimulationResult": | ||
| """ | ||
| Create SimulationResult from dotdict returned by parse_executable_output. | ||
|
|
||
| Args: | ||
| data: Dictionary containing simulation results from HDF5 file | ||
|
|
||
| Returns: | ||
| SimulationResult instance with all available fields populated | ||
| """ | ||
| return cls( | ||
| # Grid information | ||
| Nx=int(data.get("Nx", 0)), | ||
| Ny=int(data.get("Ny", 0)), | ||
| Nz=int(data.get("Nz", 0)), | ||
| Nt=int(data.get("Nt", 0)), | ||
| pml_x_size=int(data.get("pml_x_size", 0)), | ||
| pml_y_size=int(data.get("pml_y_size", 0)), | ||
| pml_z_size=int(data.get("pml_z_size", 0)), | ||
| axisymmetric_flag=bool(data.get("axisymmetric_flag", False)), | ||
| # Pressure fields | ||
| p_raw=data.get("p_raw"), | ||
| p_max=data.get("p_max"), | ||
| p_min=data.get("p_min"), | ||
| p_rms=data.get("p_rms"), | ||
| p_max_all=data.get("p_max_all"), | ||
| p_min_all=data.get("p_min_all"), | ||
| p_final=data.get("p_final"), | ||
| # Velocity fields | ||
| u_raw=data.get("u_raw"), | ||
| u_max=data.get("u_max"), | ||
| u_min=data.get("u_min"), | ||
| u_rms=data.get("u_rms"), | ||
| u_max_all=data.get("u_max_all"), | ||
| u_min_all=data.get("u_min_all"), | ||
| u_final=data.get("u_final"), | ||
| u_non_staggered_raw=data.get("u_non_staggered_raw"), | ||
| # Intensity fields | ||
| I_avg=data.get("I_avg"), | ||
| I=data.get("I"), | ||
| ) |
There was a problem hiding this comment.
_crop_pml in executor.py iterates over individual velocity-component field names (ux_final, uy_final, uz_final, ux_max_all, uy_max_all, uz_max_all, ux_min_all, uy_min_all, uz_min_all) because those are the keys the kWave C++ binary actually writes to the HDF5 output. from_dotdict maps u_final, u_max_all, and u_min_all instead — keys that are absent from the real output. data.get("u_final") therefore always returns None, and the cropped velocity data (already modified in the dotdict by _crop_pml) is silently discarded. Any user recording velocity fields will receive a SimulationResult where all those fields are None.
| @@ -85,7 +81,6 @@ class SimulationOptions(object): | |||
| """ | |||
|
|
|||
| simulation_type: SimulationType = SimulationType.FLUID | |||
There was a problem hiding this comment.
cart_interp removal is a hard breaking change
Removing cart_interp as a named constructor argument means any existing caller passing SimulationOptions(cart_interp="nearest") will get a TypeError at runtime. The PR description's claim of "non-breaking at call sites" applies only to the return-type change; the option rename is explicitly a breaking change for anyone not on the new API. Documenting a migration path or providing a __post_init__ deprecation path (accepting cart_interp and forwarding it with a warning) would be safer for library consumers.
waltsims
force-pushed
the
chore/minor-code-improvements
branch
from
4daf678 to
82a8d7f
Compare
|
Rebased onto master to resolve conflicts (the conflict was purely import-order on |
…fix)
The binary's CLI flag is --p_raw but the HDF5 output dataset is /p — the
"_raw" wording only exists in the C++ CLI surface, never in the actual
data the wrapper reads back. The original PR field names (p_raw, u_raw,
u_non_staggered_raw) leaked an implementation detail and also broke the
existing read path: from_dotdict was looking up data["p_raw"] but the
parsed dict contains data["p"], so p_raw was always None after a real
binary run (no tests caught this because they mock the parse output).
Renamed:
- p_raw -> p
- u_raw -> u
- u_non_staggered_raw -> u_non_staggered
This matches the existing sensor.record API (sensor.record = ("p",))
and the previous dict return shape, so every existing caller doing
result["p"] keeps working.
Co-Authored-By: faridyagubbayli <faridyagubbayli@gmail.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Pushed two fixes (45967ab): 1. Field naming aligned with sensor.record keys (drops the
This also fixes a latent bug: the binary's HDF5 dataset is named 2. Velocity naming uses the modern convention from the Python solver:
The legacy C++ binary names this backwards (its cc @faridyagubbayli — please flag if you'd prefer a different mapping. 70/70 unit tests pass locally. Re-triggering Greptile. @greptile re-review |
| def __getitem__(self, key: str): | ||
| """ | ||
| Enable dictionary-style access for backward compatibility. | ||
|
|
||
| Args: | ||
| key: Field name to access | ||
|
|
||
| Returns: | ||
| Value of the field | ||
|
|
||
| Raises: | ||
| KeyError: If the field does not exist | ||
| """ | ||
| if hasattr(self, key): | ||
| return getattr(self, key) | ||
| raise KeyError(f"'{key}' field not found in SimulationResult") | ||
|
|
||
| def __contains__(self, key: str) -> bool: | ||
| """ | ||
| Enable dictionary-style membership testing for backward compatibility. | ||
|
|
||
| Args: | ||
| key: Field name to check | ||
|
|
||
| Returns: | ||
| True if the field exists, False otherwise | ||
| """ | ||
| return hasattr(self, key) |
There was a problem hiding this comment.
.get() callers
SimulationResult implements __getitem__ and __contains__ for backward compatibility, but omits get(), keys(), items(), values(), and __iter__. Any existing code that calls result.get("p", None), iterates for k in result, or calls result.keys() will raise AttributeError at runtime — a silent regression for users migrating from the old dotdict return type. The PR description claims the return-type change is non-breaking, but without these methods the compatibility surface is materially incomplete.
3 participants
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
Greptile Summary
This PR consolidates the duplicate
cart_interp/cartesian_interpoption (keeping onlycartesian_interp) and replaces the untypeddotdictreturn fromExecutor.run_simulationwith a structuredSimulationResultdataclass that provides IDE completion and typed access to all sensor output fields.SimulationResultis introduced inkwave/data.pywithfrom_dotdict()that translates binary HDF5 dataset names (including the inverted velocity naming convention where binaryu= staggered, binaryu_non_staggered= collocated) to a consistent user-facing API, plus backward-compat__getitem__/__contains__methods.Executor.run_simulationnow callsfrom_dotdictafter optionally cropping the PML, and the threekspaceFirstOrder*wrapper modules update their return annotations toOptional[SimulationResult].isinstance(sensor_data, SimulationResult)and to swapcart_interpforcartesian_interp.Confidence Score: 3/5
Not safe to merge yet — velocity component fields recorded through the binary backend will silently resolve to None, and existing callers using cart_interp will get a TypeError at runtime without a migration path.
Two defects in the core data path remain unresolved from earlier review rounds. First, from_dotdict looks up combined keys (u_final, u_max_all, u_min_all) while the C++ binary writes per-component HDF5 datasets (ux_final/uy_final/uz_final, ux_max_all/uy_max_all/uz_max_all, etc.) — every user recording velocity _final or _all fields gets silently None in the result. Second, cart_interp was hard-deleted without a deprecation shim, breaking all existing call sites that pass it as a keyword argument.
kwave/data.py (from_dotdict velocity key mappings) and kwave/options/simulation_options.py (cart_interp removal without backward-compat shim)
Important Files Changed
Reviews (4): Last reviewed commit: "SimulationResult: align field names with..." | Re-trigger Greptile