Fix: Stop mutation of source and sensor by kspaceFirstOrder #614
Conversation
The test file added in the previous commit for bug #600 (test_kspaceFirstOrder3D_state.py) contained a SyntaxError due to erroneous markdown backticks (```) at the end of the file. This commit removes the offending backticks to make the test file syntactically correct.
waltsims
changed the title
|
Closes #600. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #614 +/- ##
==========================================
+ Coverage 73.95% 74.19% +0.23%
==========================================
Files 50 50
Lines 7000 7003 +3
Branches 1338 1338
==========================================
+ Hits 5177 5196 +19
+ Misses 1280 1260 -20
- Partials 543 547 +4
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:
|
WalkthroughDeep copies of Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test
participant API as kspaceFirstOrder3D
participant Sim as kWaveSimulation
participant OrigSrc as source (original)
participant OrigSen as sensor (original)
Test->>API: call kspaceFirstOrder3D(source, sensor, ...)
API->>Sim: init with source, sensor, options
Note over Sim: preserve originals and operate on copies
Sim->>Sim: self.original_source = source\nself.original_sensor = sensor
Sim->>Sim: self.source = deepcopy(source)\nself.sensor = deepcopy(sensor)
Sim->>Sim: _is_binary_sensor_mask() -- (fixed pml_size check)
Sim-->>API: run using deep-copied objects
API-->>Test: return results (originals unchanged)
Test->>API: second call with same original objects (new paths)
API->>Sim: init new kWaveSimulation(...)
Sim->>Sim: repeat preserve + deepcopy steps
Sim-->>API: run using copies
API-->>Test: return results (originals still unchanged)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
✨ Finishing touches
🧪 Generate unit tests
Comment |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kwave/kWaveSimulation.py (1)
1523-1537: Handle scalar and sequence pml_size to avoid AttributeError/TypeError.
As written, pml_size may be an int (common in options), causing len(...) to fail or pml_size.x to be missing when it’s a list/ndarray. Normalize to a Vector before use.Apply this diff to make the method robust:
- if (pml_size := self.options.pml_size) is None: + pml_size_val = self.options.pml_size + if pml_size_val is None: return False - - if len(pml_size) == 1: - # make pml_size a vector type - pml_size = Vector(np.tile(pml_size, kgrid_dim)) + # Normalize pml_size to a Vector of length kgrid_dim + arr = np.atleast_1d(np.asarray(pml_size_val)).astype(int).ravel() + if arr.size == 1: + pml_size = Vector(np.tile(int(arr.item()), kgrid_dim)) + elif arr.size == kgrid_dim: + pml_size = Vector(arr.tolist()) + else: + # Unexpected shape; cannot infer expected sensor shape + return False
🧹 Nitpick comments (4)
kwave/kWaveSimulation.py (1)
42-45: Defensive deep copies preserve caller inputs; consider memory/perf trade-offs.
This prevents mutation across runs. For large 3D arrays, deepcopy can double memory. If needed later, consider a targeted copy strategy (copy only fields that are mutated) or implementing deepcopy on kSource/kSensor to control what’s copied.tests/test_kspaceFirstOrder3D_state.py (3)
41-44: Use boolean dtype for a binary sensor mask.
This clarifies intent and avoids implicit float->bool comparisons later.- sensor_mask_array = np.zeros(N) + sensor_mask_array = np.zeros(N, dtype=bool) sensor_mask_array[0, :, :] = 1 # Corrected to be a plane for 3D
96-99: Prefer NumPy test helpers for clearer diffs on failure.
np.testing.assert_array_equal gives better diagnostics than array_equal.- assert np.array_equal(source.p0, original_source_p0), "source.p0 was modified after first run" - assert np.array_equal(sensor.mask, original_sensor_mask), "sensor.mask was modified after first run" + np.testing.assert_array_equal(source.p0, original_source_p0, err_msg="source.p0 was modified after first run") + np.testing.assert_array_equal(sensor.mask, original_sensor_mask, err_msg="sensor.mask was modified after first run")
124-125: Same here: use NumPy assertions for the final check.- assert np.array_equal(source.p0, original_source_p0), "source.p0 was modified after second run" - assert np.array_equal(sensor.mask, original_sensor_mask), "sensor.mask was modified after second run" + np.testing.assert_array_equal(source.p0, original_source_p0, err_msg="source.p0 was modified after second run") + np.testing.assert_array_equal(sensor.mask, original_sensor_mask, err_msg="sensor.mask was modified after second run")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
kwave/kWaveSimulation.py(3 hunks)tests/test_kspaceFirstOrder3D_state.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_kspaceFirstOrder3D_state.py (8)
kwave/data.py (1)
Vector(7-49)kwave/ksensor.py (1)
kSensor(9-81)kwave/ksource.py (1)
kSource(11-382)kwave/kspaceFirstOrder3D.py (1)
kspaceFirstOrder3D(132-376)kwave/options/simulation_execution_options.py (1)
SimulationExecutionOptions(13-308)kwave/options/simulation_options.py (1)
SimulationOptions(46-350)kwave/utils/filters.py (1)
smooth(549-601)kwave/utils/mapgen.py (1)
make_ball(505-554)
⏰ 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). (29)
- GitHub Check: collect_references
- GitHub Check: test_install (macos-latest, 3.13, all)
- GitHub Check: test_install (ubuntu-latest, 3.13, dev)
- GitHub Check: test_install (ubuntu-latest, 3.12, all)
- GitHub Check: test_install (ubuntu-latest, 3.13, docs)
- GitHub Check: test_install (ubuntu-latest, 3.13, test)
- GitHub Check: test_install (ubuntu-latest, 3.10, all)
- GitHub Check: test_install (ubuntu-latest, 3.11, dev)
- GitHub Check: test_install (ubuntu-latest, 3.12, examples)
- GitHub Check: test_install (windows-latest, 3.12, examples)
- GitHub Check: test_install (ubuntu-latest, 3.11, test)
- GitHub Check: test_install (windows-latest, 3.12, dev)
- GitHub Check: test_install (windows-latest, 3.13, all)
- GitHub Check: test_install (windows-latest, 3.12, docs)
- GitHub Check: test_install (windows-latest, 3.12, test)
- GitHub Check: test_install (windows-latest, 3.11, docs)
- GitHub Check: test_example (macos-latest, 3.11)
- GitHub Check: test_example (macos-latest, 3.13)
- GitHub Check: test_example (macos-latest, 3.10)
- GitHub Check: test_example (ubuntu-latest, 3.11)
- GitHub Check: test_example (macos-latest, 3.12)
- GitHub Check: test_example (ubuntu-latest, 3.12)
- GitHub Check: test_example (ubuntu-latest, 3.13)
- GitHub Check: test_example (windows-latest, 3.13)
- GitHub Check: test_example (windows-latest, 3.10)
- GitHub Check: test_example (windows-latest, 3.11)
- GitHub Check: test_example (ubuntu-latest, 3.10)
- GitHub Check: test_example (windows-latest, 3.12)
- GitHub Check: collect_references
🔇 Additional comments (1)
kwave/kWaveSimulation.py (1)
3-3: Import of deepcopy is fine.
No concerns with adding this import.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kwave/kWaveSimulation.py (1)
1523-1529: len() on scalar pml_size can raise TypeError; normalize robustly.
If SimulationOptions.pml_size is an int (as in the new test), len(pml_size) will crash. Normalize scalars/sequences to Vector first.Apply:
- if (pml_size := self.options.pml_size) is None: + pml_size = self.options.pml_size + if pml_size is None: return False - if len(pml_size) == 1: - # make pml_size a vector type - pml_size = Vector(np.tile(pml_size, kgrid_dim)) + # Normalize to a Vector: accept scalars, 1-elem sequences, or full vectors + if np.isscalar(pml_size): + pml_size = Vector([int(pml_size)] * kgrid_dim) + else: + try: + seq = list(pml_size) + if len(seq) == 1: + pml_size = Vector([int(seq[0])] * kgrid_dim) + else: + pml_size = Vector(seq) + except TypeError: + pml_size = Vector([int(pml_size)] * kgrid_dim)
♻️ Duplicate comments (1)
tests/test_kspaceFirstOrder3D_state.py (1)
49-57: Pass PML size as 3-vector; scalar can break option handling.
Repeat of prior feedback: ensure option_factory/indexing logic sees a length-3 sequence.Apply:
- simulation_options = SimulationOptions( + simulation_options = SimulationOptions( save_to_disk=True, # Must be true for kspaceFirstOrder3D - pml_size=PML_size, + pml_size=[PML_size] * 3, pml_inside=False, smooth_p0=False, # p0 is already smoothed data_cast="single", - input_filename=input_filename, - output_filename=output_filename, + input_filename=str(input_filename), + output_filename=str(output_filename), )
🧹 Nitpick comments (5)
kwave/kWaveSimulation.py (1)
42-46: Defensive copies of source/sensor prevent input mutation.
Solid fix for #600. Minor: this doubles peak memory for large arrays; consider an opt-in flag (e.g., copy_inputs=True) or a custom deepcopy on kSource/kSensor to avoid copying immutable fields.tests/test_kspaceFirstOrder3D_state.py (4)
33-35: Use integer center for make_ball to avoid implicit float handling.
Ensures indices are integral across backends.Apply:
- p0_array = ball_magnitude * make_ball(N, N / 2, ball_radius) + p0_array = ball_magnitude * make_ball(N, N // 2, ball_radius)
41-43: Use boolean dtype for a binary mask.
Avoids float sums and makes intent explicit.Apply:
- sensor_mask_array = np.zeros(N) - sensor_mask_array[0, :, :] = 1 # Corrected to be a plane for 3D + sensor_mask_array = np.zeros(N, dtype=bool) + sensor_mask_array[0, :, :] = True # Planar sensor in 3D
61-66: Path-like for checkpoint_file — cast to str for CLI safety.
Prevents downstream subprocess arg type issues on older Python/OS combos.Apply:
- execution_options = SimulationExecutionOptions( + execution_options = SimulationExecutionOptions( is_gpu_simulation=False, # Assuming CPU for basic test - checkpoint_file=checkpoint_filename, + checkpoint_file=str(checkpoint_filename), checkpoint_timesteps=checkpoint_timesteps, verbose_level=0, # Keep test output clean )
107-110: Also cast run-2 checkpoint path to str.
Consistency with above.Apply:
- if execution_options_run2.checkpoint_file: # Only change if it exists - execution_options_run2.checkpoint_file = tmpdir / "kwave_checkpoint_run2.h5" + if execution_options_run2.checkpoint_file: # Only change if it exists + execution_options_run2.checkpoint_file = str(tmpdir / "kwave_checkpoint_run2.h5")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
kwave/kWaveSimulation.py(3 hunks)tests/test_kspaceFirstOrder3D_state.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: pytest
tests/test_kspaceFirstOrder3D_state.py
[error] 94-94: First call to kspaceFirstOrder3D failed: External binary 'kspaceFirstOrder-OMP' terminated with SIGABRT during test_kspaceFirstOrder3D_input_state_preservation. Command: '/Users/runner/work/k-wave-python/k-wave-python/kwave/bin/darwin/kspaceFirstOrder-OMP' -i '/var/folders/x7/ch5v91h56_zbvbd1y2f600dm0000gn/T/tmp8ruc7ke7/kwave_input.h5' -o '/var/folders/x7/ch5v91h56_zbvbd1y2f600dm0000gn/T/tmp8ruc7ke7/kwave_output.h5' -t 3 --checkpoint_timesteps 300 --checkpoint_file '/var/folders/x7/ch5v91h56_zbvbd1y2f600dm0000gn/T/tmp8ruc7ke7/kwave_checkpoint.h5' --p_raw -s 1'. The underlying error: dyld: Library not loaded: /opt/homebrew/opt/fftw/lib/libfftw3f.3.dylib (no such file).
🔇 Additional comments (1)
kwave/kWaveSimulation.py (1)
3-3: Importing deepcopy to protect inputs — good call.
This enables safe internal mutation without leaking to caller state.
Docstrings generation was requested by @waltsims. * #614 (comment) The following files were modified: * `kwave/kWaveSimulation.py` * `tests/test_kspaceFirstOrder3D_state.py`
|
Note Generated docstrings for this pull request at #639 |
The test file added in the previous commit for bug #600 (test_kspaceFirstOrder3D_state.py) contained a SyntaxError due to erroneous markdown backticks (```) at the end of the file.
This commit removes the offending backticks to make the test file syntactically correct.
Summary by CodeRabbit
Bug Fixes
Tests