From 51a3f824ed534e1e163564967991e6a11b65cfb3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 12 May 2026 00:27:09 +0000 Subject: [PATCH 1/8] Fix binary_path dropped by options_to_kwargs() (issue #725) Agent-Logs-Url: https://github.com/waltsims/k-wave-python/sessions/2b340e29-96d4-4795-be98-d80ae80e5125 Co-authored-by: waltsims <8669206+waltsims@users.noreply.github.com> --- kwave/compat.py | 2 ++ kwave/kspaceFirstOrder.py | 6 +++++- kwave/solvers/cpp_simulation.py | 24 +++++++++++++++--------- tests/test_compat.py | 5 +++++ 4 files changed, 27 insertions(+), 10 deletions(-) diff --git a/kwave/compat.py b/kwave/compat.py index 7dcb074e9..d495d6582 100644 --- a/kwave/compat.py +++ b/kwave/compat.py @@ -70,5 +70,7 @@ def options_to_kwargs(simulation_options=None, execution_options=None): kwargs["num_threads"] = opts.num_threads if opts.device_num is not None: kwargs["device_num"] = opts.device_num + if opts._binary_path is not None: + kwargs["binary_path"] = opts._binary_path return kwargs diff --git a/kwave/kspaceFirstOrder.py b/kwave/kspaceFirstOrder.py index 3a71cccd7..b10c07674 100644 --- a/kwave/kspaceFirstOrder.py +++ b/kwave/kspaceFirstOrder.py @@ -102,6 +102,7 @@ def kspaceFirstOrder( debug: bool = False, num_threads: Optional[int] = None, device_num: Optional[int] = None, + binary_path: Optional[str] = None, ) -> dict: """Run a k-Wave simulation. @@ -154,6 +155,9 @@ def kspaceFirstOrder( num_threads: Thread count for the C++ OMP binary. ``None`` uses all available cores. Default ``None``. device_num: GPU device index for CUDA execution. Default ``None``. + binary_path: Path to a custom C++ binary. When ``None`` (default), + the binary bundled with ``k-wave-data`` is used. Only applies + when ``backend="cpp"``. Returns: dict: Recorded sensor data keyed by field name (e.g. @@ -247,7 +251,7 @@ def kspaceFirstOrder( if not pml_inside: result["pml_size"] = pml_size return result - result = cpp_sim.run(device=device, num_threads=num_threads, device_num=device_num, quiet=quiet, debug=debug, data_path=data_path) + result = cpp_sim.run(device=device, num_threads=num_threads, device_num=device_num, quiet=quiet, debug=debug, data_path=data_path, binary_path=binary_path) # --- Post-processing: strip PML from full-grid fields --- diff --git a/kwave/solvers/cpp_simulation.py b/kwave/solvers/cpp_simulation.py index 545898b4d..c0028f0ba 100644 --- a/kwave/solvers/cpp_simulation.py +++ b/kwave/solvers/cpp_simulation.py @@ -9,6 +9,7 @@ import stat import subprocess import tempfile +from pathlib import Path import numpy as np @@ -48,14 +49,14 @@ def prepare(self, data_path=None): self._write_hdf5(input_file) return input_file, output_file - def run(self, *, device="cpu", num_threads=None, device_num=None, quiet=False, debug=False, data_path=None): + def run(self, *, device="cpu", num_threads=None, device_num=None, quiet=False, debug=False, data_path=None, binary_path=None): import warnings cleanup = data_path is None input_file, output_file = self.prepare(data_path=data_path) data_dir = os.path.dirname(input_file) try: - self._execute(input_file, output_file, device=device, num_threads=num_threads, device_num=device_num, quiet=quiet, debug=debug) + self._execute(input_file, output_file, device=device, num_threads=num_threads, device_num=device_num, quiet=quiet, debug=debug, binary_path=binary_path) result = self._parse_output(output_file) result = self._fix_output_order(result) return result @@ -311,16 +312,21 @@ def _get_absorbing_flag(self): return 2 # Stokes return 1 # Power-law - def _execute(self, input_file, output_file, *, device, num_threads, device_num, quiet, debug): + def _execute(self, input_file, output_file, *, device, num_threads, device_num, quiet, debug, binary_path=None): """Run the C++ k-Wave binary.""" import kwave - binary_name = "kspaceFirstOrder-CUDA" if device == "gpu" else "kspaceFirstOrder-OMP" - binary_path = kwave.BINARY_PATH / binary_name - if not binary_path.exists(): - if kwave.PLATFORM == "darwin" and device == "gpu": - raise ValueError("GPU simulations are not supported on macOS. Use device='cpu'.") - raise FileNotFoundError(f"C++ binary not found at {binary_path}. Install with: pip install k-wave-data") + if binary_path is not None: + binary_path = Path(binary_path) + if not binary_path.exists(): + raise FileNotFoundError(f"Custom C++ binary not found at {binary_path}.") + else: + binary_name = "kspaceFirstOrder-CUDA" if device == "gpu" else "kspaceFirstOrder-OMP" + binary_path = kwave.BINARY_PATH / binary_name + if not binary_path.exists(): + if kwave.PLATFORM == "darwin" and device == "gpu": + raise ValueError("GPU simulations are not supported on macOS. Use device='cpu'.") + raise FileNotFoundError(f"C++ binary not found at {binary_path}. Install with: pip install k-wave-data") binary_path.chmod(binary_path.stat().st_mode | stat.S_IEXEC) # Build command-line options diff --git a/tests/test_compat.py b/tests/test_compat.py index 63d6d2519..498b20017 100644 --- a/tests/test_compat.py +++ b/tests/test_compat.py @@ -71,6 +71,11 @@ def test_device_num(self): kwargs = options_to_kwargs(execution_options=opts) assert kwargs["device_num"] == 1 + def test_binary_path(self): + opts = SimulationExecutionOptions(is_gpu_simulation=False, backend="OMP", binary_path="./kspaceFirstOrder-OMP") + kwargs = options_to_kwargs(execution_options=opts) + assert kwargs.get("binary_path") == "./kspaceFirstOrder-OMP" + class TestCombined: def test_both_options(self): From 5961b2755f49264d591f56cb5db69192665e050f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 12 May 2026 00:28:33 +0000 Subject: [PATCH 2/8] Add explanatory comment for _binary_path access in options_to_kwargs Agent-Logs-Url: https://github.com/waltsims/k-wave-python/sessions/2b340e29-96d4-4795-be98-d80ae80e5125 Co-authored-by: waltsims <8669206+waltsims@users.noreply.github.com> --- kwave/compat.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/kwave/compat.py b/kwave/compat.py index d495d6582..d88834019 100644 --- a/kwave/compat.py +++ b/kwave/compat.py @@ -70,6 +70,9 @@ def options_to_kwargs(simulation_options=None, execution_options=None): kwargs["num_threads"] = opts.num_threads if opts.device_num is not None: kwargs["device_num"] = opts.device_num + # Use _binary_path (not the property) because the property always returns + # a non-None default path, making it impossible to distinguish "user-set" + # from "auto-resolved default" via the public API. if opts._binary_path is not None: kwargs["binary_path"] = opts._binary_path From 6064d4eababb8b71759133b6e50f80a45e979492 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 12 May 2026 01:17:38 +0000 Subject: [PATCH 3/8] Refactor binary path resolution to _resolve_binary_path() and add tests Agent-Logs-Url: https://github.com/waltsims/k-wave-python/sessions/fd9e1ab3-1448-4f01-b58c-752897955bcc Co-authored-by: waltsims <8669206+waltsims@users.noreply.github.com> --- kwave/solvers/cpp_simulation.py | 50 ++++++++++++++++----- tests/test_cpp_simulation.py | 77 +++++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 12 deletions(-) create mode 100644 tests/test_cpp_simulation.py diff --git a/kwave/solvers/cpp_simulation.py b/kwave/solvers/cpp_simulation.py index c0028f0ba..ea6facdc2 100644 --- a/kwave/solvers/cpp_simulation.py +++ b/kwave/solvers/cpp_simulation.py @@ -312,21 +312,47 @@ def _get_absorbing_flag(self): return 2 # Stokes return 1 # Power-law - def _execute(self, input_file, output_file, *, device, num_threads, device_num, quiet, debug, binary_path=None): - """Run the C++ k-Wave binary.""" + @staticmethod + def _resolve_binary_path(device: str, binary_path=None) -> Path: + """Resolve the path to the C++ k-Wave binary. + + When *binary_path* is provided the caller's custom binary is used and + verified to exist. Otherwise the bundled binary is selected based on + *device* (``"gpu"`` → CUDA, anything else → OMP) and verified to exist. + + Args: + device: ``"cpu"`` or ``"gpu"``. + binary_path: Optional path to a custom binary. When ``None`` the + bundled binary that ships with ``k-wave-data`` is used. + + Returns: + Resolved :class:`~pathlib.Path` pointing at the binary. + + Raises: + FileNotFoundError: When the binary does not exist at the resolved + path. + ValueError: When ``device="gpu"`` is requested on macOS where no + CUDA binary is available. + """ import kwave if binary_path is not None: - binary_path = Path(binary_path) - if not binary_path.exists(): - raise FileNotFoundError(f"Custom C++ binary not found at {binary_path}.") - else: - binary_name = "kspaceFirstOrder-CUDA" if device == "gpu" else "kspaceFirstOrder-OMP" - binary_path = kwave.BINARY_PATH / binary_name - if not binary_path.exists(): - if kwave.PLATFORM == "darwin" and device == "gpu": - raise ValueError("GPU simulations are not supported on macOS. Use device='cpu'.") - raise FileNotFoundError(f"C++ binary not found at {binary_path}. Install with: pip install k-wave-data") + resolved = Path(binary_path) + if not resolved.exists(): + raise FileNotFoundError(f"Custom C++ binary not found at {resolved}.") + return resolved + + binary_name = "kspaceFirstOrder-CUDA" if device == "gpu" else "kspaceFirstOrder-OMP" + resolved = kwave.BINARY_PATH / binary_name + if not resolved.exists(): + if kwave.PLATFORM == "darwin" and device == "gpu": + raise ValueError("GPU simulations are not supported on macOS. Use device='cpu'.") + raise FileNotFoundError(f"C++ binary not found at {resolved}. Install with: pip install k-wave-data") + return resolved + + def _execute(self, input_file, output_file, *, device, num_threads, device_num, quiet, debug, binary_path=None): + """Run the C++ k-Wave binary.""" + binary_path = self._resolve_binary_path(device, binary_path) binary_path.chmod(binary_path.stat().st_mode | stat.S_IEXEC) # Build command-line options diff --git a/tests/test_cpp_simulation.py b/tests/test_cpp_simulation.py new file mode 100644 index 000000000..ac972688c --- /dev/null +++ b/tests/test_cpp_simulation.py @@ -0,0 +1,77 @@ +"""Unit tests for CppSimulation._resolve_binary_path.""" +import stat +from pathlib import Path +from unittest.mock import patch + +import pytest + +from kwave.solvers.cpp_simulation import CppSimulation + + +class TestResolveBinaryPath: + """Tests for CppSimulation._resolve_binary_path().""" + + # ------------------------------------------------------------------ + # Custom binary_path provided by the caller + # ------------------------------------------------------------------ + + def test_custom_path_existing_file_is_returned(self, tmp_path): + binary = tmp_path / "my-kwave-binary" + binary.write_text("#!/bin/sh\n") + resolved = CppSimulation._resolve_binary_path("cpu", binary_path=str(binary)) + assert resolved == binary + + def test_custom_path_returns_path_object(self, tmp_path): + binary = tmp_path / "my-kwave-binary" + binary.write_text("#!/bin/sh\n") + resolved = CppSimulation._resolve_binary_path("cpu", binary_path=str(binary)) + assert isinstance(resolved, Path) + + def test_custom_path_missing_raises_file_not_found(self, tmp_path): + missing = tmp_path / "does-not-exist" + with pytest.raises(FileNotFoundError, match="Custom C\\+\\+ binary not found"): + CppSimulation._resolve_binary_path("cpu", binary_path=str(missing)) + + def test_custom_path_used_regardless_of_device(self, tmp_path): + binary = tmp_path / "my-cuda-binary" + binary.write_text("#!/bin/sh\n") + # Even when device="gpu" the custom path should be returned as-is, + # without triggering the macOS CUDA guard or any other default logic. + resolved = CppSimulation._resolve_binary_path("gpu", binary_path=str(binary)) + assert resolved == binary + + # ------------------------------------------------------------------ + # Default bundled binary (no custom path) + # ------------------------------------------------------------------ + + def test_default_cpu_selects_omp_binary(self, tmp_path): + omp_binary = tmp_path / "kspaceFirstOrder-OMP" + omp_binary.write_text("#!/bin/sh\n") + with patch("kwave.BINARY_PATH", tmp_path), patch("kwave.PLATFORM", "linux"): + resolved = CppSimulation._resolve_binary_path("cpu") + assert resolved.name == "kspaceFirstOrder-OMP" + + def test_default_gpu_selects_cuda_binary(self, tmp_path): + cuda_binary = tmp_path / "kspaceFirstOrder-CUDA" + cuda_binary.write_text("#!/bin/sh\n") + with patch("kwave.BINARY_PATH", tmp_path), patch("kwave.PLATFORM", "linux"): + resolved = CppSimulation._resolve_binary_path("gpu") + assert resolved.name == "kspaceFirstOrder-CUDA" + + def test_default_missing_omp_raises_file_not_found(self, tmp_path): + # No OMP binary in tmp_path + with patch("kwave.BINARY_PATH", tmp_path), patch("kwave.PLATFORM", "linux"): + with pytest.raises(FileNotFoundError, match="pip install k-wave-data"): + CppSimulation._resolve_binary_path("cpu") + + def test_default_missing_cuda_on_linux_raises_file_not_found(self, tmp_path): + # No CUDA binary in tmp_path on a non-macOS platform + with patch("kwave.BINARY_PATH", tmp_path), patch("kwave.PLATFORM", "linux"): + with pytest.raises(FileNotFoundError, match="pip install k-wave-data"): + CppSimulation._resolve_binary_path("gpu") + + def test_default_gpu_on_macos_raises_value_error(self, tmp_path): + # macOS + gpu → ValueError before checking binary existence + with patch("kwave.BINARY_PATH", tmp_path), patch("kwave.PLATFORM", "darwin"): + with pytest.raises(ValueError, match="not supported on macOS"): + CppSimulation._resolve_binary_path("gpu") From 818aa3c62e0aab393a54134b876dae6d77fcd84a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 12 May 2026 01:19:11 +0000 Subject: [PATCH 4/8] Address code review: add chmod test, clarify compat.py comment Agent-Logs-Url: https://github.com/waltsims/k-wave-python/sessions/fd9e1ab3-1448-4f01-b58c-752897955bcc Co-authored-by: waltsims <8669206+waltsims@users.noreply.github.com> --- kwave/compat.py | 7 ++++--- tests/test_cpp_simulation.py | 31 ++++++++++++++++++++++++++++++- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/kwave/compat.py b/kwave/compat.py index d88834019..d9c207ea1 100644 --- a/kwave/compat.py +++ b/kwave/compat.py @@ -70,9 +70,10 @@ def options_to_kwargs(simulation_options=None, execution_options=None): kwargs["num_threads"] = opts.num_threads if opts.device_num is not None: kwargs["device_num"] = opts.device_num - # Use _binary_path (not the property) because the property always returns - # a non-None default path, making it impossible to distinguish "user-set" - # from "auto-resolved default" via the public API. + # Use _binary_path (not the property) because the property always resolves + # a non-None default (kwave.BINARY_PATH / binary_name), making it impossible + # to distinguish "user-set custom path" from "auto-resolved default" via the + # public API. if opts._binary_path is not None: kwargs["binary_path"] = opts._binary_path diff --git a/tests/test_cpp_simulation.py b/tests/test_cpp_simulation.py index ac972688c..d59328f41 100644 --- a/tests/test_cpp_simulation.py +++ b/tests/test_cpp_simulation.py @@ -9,7 +9,12 @@ class TestResolveBinaryPath: - """Tests for CppSimulation._resolve_binary_path().""" + """Tests for CppSimulation._resolve_binary_path(). + + Note: _resolve_binary_path() is responsible only for locating the binary. + Making it executable (chmod +x) is done in _execute() after the path is + resolved, keeping the two concerns separate. + """ # ------------------------------------------------------------------ # Custom binary_path provided by the caller @@ -75,3 +80,27 @@ def test_default_gpu_on_macos_raises_value_error(self, tmp_path): with patch("kwave.BINARY_PATH", tmp_path), patch("kwave.PLATFORM", "darwin"): with pytest.raises(ValueError, match="not supported on macOS"): CppSimulation._resolve_binary_path("gpu") + + # ------------------------------------------------------------------ + # Executable permission is set by _execute() after path resolution + # ------------------------------------------------------------------ + + def test_execute_makes_binary_executable(self, tmp_path, monkeypatch): + """_execute() sets the executable bit on the resolved binary.""" + import subprocess + + binary = tmp_path / "kspaceFirstOrder-OMP" + binary.write_bytes(b"") + # Remove executable bit so we can verify _execute() sets it + binary.chmod(binary.stat().st_mode & ~(stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH)) + assert not (binary.stat().st_mode & stat.S_IEXEC) + + # Patch _resolve_binary_path so it returns our binary without querying kwave paths + monkeypatch.setattr(CppSimulation, "_resolve_binary_path", staticmethod(lambda device, binary_path=None: binary)) + # Patch subprocess.run so no real process is spawned + monkeypatch.setattr(subprocess, "run", lambda *a, **kw: None) + + sim = CppSimulation.__new__(CppSimulation) + sim._execute("input.h5", "output.h5", device="cpu", num_threads=None, device_num=None, quiet=False, debug=False) + + assert binary.stat().st_mode & stat.S_IEXEC From 627eab7069bcaa111f32427f312529e5f78a3c16 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 12 May 2026 01:20:22 +0000 Subject: [PATCH 5/8] Move subprocess import to top-level, add type annotation to _resolve_binary_path Agent-Logs-Url: https://github.com/waltsims/k-wave-python/sessions/fd9e1ab3-1448-4f01-b58c-752897955bcc Co-authored-by: waltsims <8669206+waltsims@users.noreply.github.com> --- kwave/solvers/cpp_simulation.py | 2 +- tests/test_cpp_simulation.py | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/kwave/solvers/cpp_simulation.py b/kwave/solvers/cpp_simulation.py index ea6facdc2..6151db6c0 100644 --- a/kwave/solvers/cpp_simulation.py +++ b/kwave/solvers/cpp_simulation.py @@ -313,7 +313,7 @@ def _get_absorbing_flag(self): return 1 # Power-law @staticmethod - def _resolve_binary_path(device: str, binary_path=None) -> Path: + def _resolve_binary_path(device: str, binary_path: str | None = None) -> Path: """Resolve the path to the C++ k-Wave binary. When *binary_path* is provided the caller's custom binary is used and diff --git a/tests/test_cpp_simulation.py b/tests/test_cpp_simulation.py index d59328f41..bf25f0945 100644 --- a/tests/test_cpp_simulation.py +++ b/tests/test_cpp_simulation.py @@ -1,5 +1,6 @@ """Unit tests for CppSimulation._resolve_binary_path.""" import stat +import subprocess from pathlib import Path from unittest.mock import patch @@ -87,8 +88,6 @@ def test_default_gpu_on_macos_raises_value_error(self, tmp_path): def test_execute_makes_binary_executable(self, tmp_path, monkeypatch): """_execute() sets the executable bit on the resolved binary.""" - import subprocess - binary = tmp_path / "kspaceFirstOrder-OMP" binary.write_bytes(b"") # Remove executable bit so we can verify _execute() sets it From 94098102ffa459811af37e8dca9d69d13a3b67d6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 12 May 2026 02:39:48 +0000 Subject: [PATCH 6/8] Move import kwave inside default-path branch per reviewer suggestion Agent-Logs-Url: https://github.com/waltsims/k-wave-python/sessions/5cd6f852-070e-4b38-b06a-39410dafaecb Co-authored-by: waltsims <8669206+waltsims@users.noreply.github.com> --- kwave/solvers/cpp_simulation.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/kwave/solvers/cpp_simulation.py b/kwave/solvers/cpp_simulation.py index 6151db6c0..90a7bc32b 100644 --- a/kwave/solvers/cpp_simulation.py +++ b/kwave/solvers/cpp_simulation.py @@ -334,14 +334,14 @@ def _resolve_binary_path(device: str, binary_path: str | None = None) -> Path: ValueError: When ``device="gpu"`` is requested on macOS where no CUDA binary is available. """ - import kwave - if binary_path is not None: resolved = Path(binary_path) if not resolved.exists(): raise FileNotFoundError(f"Custom C++ binary not found at {resolved}.") return resolved + import kwave + binary_name = "kspaceFirstOrder-CUDA" if device == "gpu" else "kspaceFirstOrder-OMP" resolved = kwave.BINARY_PATH / binary_name if not resolved.exists(): From 3f48987036595997c9b8b578fdfa7126b65bdcc9 Mon Sep 17 00:00:00 2001 From: Walter Simson Date: Sat, 16 May 2026 15:58:45 +0000 Subject: [PATCH 7/8] Apply /simplify findings: rename shadowed binary_path, trim comments - _execute(): rename the local from binary_path to resolved so the parameter (str | None) and the resolved Path don't share a name. - compat.py: collapse the WHY-comment on _binary_path access to 2 lines. - test_cpp_simulation.py: fold redundant test_custom_path_returns_path_object into test_custom_path_existing_file_is_returned (equality check already implies the Path type), drop section-separator comments, drop WHAT-only comments inside tests. Co-Authored-By: Claude Opus 4.7 (1M context) --- kwave/compat.py | 6 ++---- kwave/solvers/cpp_simulation.py | 6 +++--- tests/test_cpp_simulation.py | 35 +++++---------------------------- 3 files changed, 10 insertions(+), 37 deletions(-) diff --git a/kwave/compat.py b/kwave/compat.py index d9c207ea1..a077c8fcf 100644 --- a/kwave/compat.py +++ b/kwave/compat.py @@ -70,10 +70,8 @@ def options_to_kwargs(simulation_options=None, execution_options=None): kwargs["num_threads"] = opts.num_threads if opts.device_num is not None: kwargs["device_num"] = opts.device_num - # Use _binary_path (not the property) because the property always resolves - # a non-None default (kwave.BINARY_PATH / binary_name), making it impossible - # to distinguish "user-set custom path" from "auto-resolved default" via the - # public API. + # Read _binary_path directly: the property auto-resolves to a default, + # so it can't distinguish a user-set path from one. if opts._binary_path is not None: kwargs["binary_path"] = opts._binary_path diff --git a/kwave/solvers/cpp_simulation.py b/kwave/solvers/cpp_simulation.py index 90a7bc32b..fe465c7ca 100644 --- a/kwave/solvers/cpp_simulation.py +++ b/kwave/solvers/cpp_simulation.py @@ -352,8 +352,8 @@ def _resolve_binary_path(device: str, binary_path: str | None = None) -> Path: def _execute(self, input_file, output_file, *, device, num_threads, device_num, quiet, debug, binary_path=None): """Run the C++ k-Wave binary.""" - binary_path = self._resolve_binary_path(device, binary_path) - binary_path.chmod(binary_path.stat().st_mode | stat.S_IEXEC) + resolved = self._resolve_binary_path(device, binary_path) + resolved.chmod(resolved.stat().st_mode | stat.S_IEXEC) # Build command-line options options = ["-i", input_file, "-o", output_file] @@ -368,7 +368,7 @@ def _execute(self, input_file, output_file, *, device, num_threads, device_num, elif debug: options.extend(["--verbose", "2"]) - command = [str(binary_path)] + options + command = [str(resolved)] + options try: subprocess.run(command, capture_output=quiet, text=True, check=True) except subprocess.CalledProcessError as e: diff --git a/tests/test_cpp_simulation.py b/tests/test_cpp_simulation.py index bf25f0945..bbbaa900a 100644 --- a/tests/test_cpp_simulation.py +++ b/tests/test_cpp_simulation.py @@ -12,26 +12,16 @@ class TestResolveBinaryPath: """Tests for CppSimulation._resolve_binary_path(). - Note: _resolve_binary_path() is responsible only for locating the binary. - Making it executable (chmod +x) is done in _execute() after the path is - resolved, keeping the two concerns separate. + chmod is exercised separately in test_execute_makes_binary_executable + because it lives in _execute(), not the resolver. """ - # ------------------------------------------------------------------ - # Custom binary_path provided by the caller - # ------------------------------------------------------------------ - def test_custom_path_existing_file_is_returned(self, tmp_path): - binary = tmp_path / "my-kwave-binary" - binary.write_text("#!/bin/sh\n") - resolved = CppSimulation._resolve_binary_path("cpu", binary_path=str(binary)) - assert resolved == binary - - def test_custom_path_returns_path_object(self, tmp_path): binary = tmp_path / "my-kwave-binary" binary.write_text("#!/bin/sh\n") resolved = CppSimulation._resolve_binary_path("cpu", binary_path=str(binary)) assert isinstance(resolved, Path) + assert resolved == binary def test_custom_path_missing_raises_file_not_found(self, tmp_path): missing = tmp_path / "does-not-exist" @@ -41,15 +31,10 @@ def test_custom_path_missing_raises_file_not_found(self, tmp_path): def test_custom_path_used_regardless_of_device(self, tmp_path): binary = tmp_path / "my-cuda-binary" binary.write_text("#!/bin/sh\n") - # Even when device="gpu" the custom path should be returned as-is, - # without triggering the macOS CUDA guard or any other default logic. + # device="gpu" must not trigger the macOS CUDA guard when a custom path is given. resolved = CppSimulation._resolve_binary_path("gpu", binary_path=str(binary)) assert resolved == binary - # ------------------------------------------------------------------ - # Default bundled binary (no custom path) - # ------------------------------------------------------------------ - def test_default_cpu_selects_omp_binary(self, tmp_path): omp_binary = tmp_path / "kspaceFirstOrder-OMP" omp_binary.write_text("#!/bin/sh\n") @@ -65,38 +50,28 @@ def test_default_gpu_selects_cuda_binary(self, tmp_path): assert resolved.name == "kspaceFirstOrder-CUDA" def test_default_missing_omp_raises_file_not_found(self, tmp_path): - # No OMP binary in tmp_path with patch("kwave.BINARY_PATH", tmp_path), patch("kwave.PLATFORM", "linux"): with pytest.raises(FileNotFoundError, match="pip install k-wave-data"): CppSimulation._resolve_binary_path("cpu") def test_default_missing_cuda_on_linux_raises_file_not_found(self, tmp_path): - # No CUDA binary in tmp_path on a non-macOS platform with patch("kwave.BINARY_PATH", tmp_path), patch("kwave.PLATFORM", "linux"): with pytest.raises(FileNotFoundError, match="pip install k-wave-data"): CppSimulation._resolve_binary_path("gpu") def test_default_gpu_on_macos_raises_value_error(self, tmp_path): - # macOS + gpu → ValueError before checking binary existence + # macOS + gpu must raise before the binary-existence check. with patch("kwave.BINARY_PATH", tmp_path), patch("kwave.PLATFORM", "darwin"): with pytest.raises(ValueError, match="not supported on macOS"): CppSimulation._resolve_binary_path("gpu") - # ------------------------------------------------------------------ - # Executable permission is set by _execute() after path resolution - # ------------------------------------------------------------------ - def test_execute_makes_binary_executable(self, tmp_path, monkeypatch): - """_execute() sets the executable bit on the resolved binary.""" binary = tmp_path / "kspaceFirstOrder-OMP" binary.write_bytes(b"") - # Remove executable bit so we can verify _execute() sets it binary.chmod(binary.stat().st_mode & ~(stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH)) assert not (binary.stat().st_mode & stat.S_IEXEC) - # Patch _resolve_binary_path so it returns our binary without querying kwave paths monkeypatch.setattr(CppSimulation, "_resolve_binary_path", staticmethod(lambda device, binary_path=None: binary)) - # Patch subprocess.run so no real process is spawned monkeypatch.setattr(subprocess, "run", lambda *a, **kw: None) sim = CppSimulation.__new__(CppSimulation) From ccf3ddbb47a33f5601d5e117b99144e2b6d02aca Mon Sep 17 00:00:00 2001 From: Walter Simson Date: Sat, 16 May 2026 16:21:02 +0000 Subject: [PATCH 8/8] Skip chmod test on Windows os.chmod(mode | S_IEXEC) is a no-op on Windows because Windows determines executability by file extension (.exe), not Unix bits. The production _execute() call is harmless on Windows but the test assertion isn't satisfiable. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/test_cpp_simulation.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_cpp_simulation.py b/tests/test_cpp_simulation.py index bbbaa900a..c21710289 100644 --- a/tests/test_cpp_simulation.py +++ b/tests/test_cpp_simulation.py @@ -1,6 +1,7 @@ """Unit tests for CppSimulation._resolve_binary_path.""" import stat import subprocess +import sys from pathlib import Path from unittest.mock import patch @@ -65,6 +66,7 @@ def test_default_gpu_on_macos_raises_value_error(self, tmp_path): with pytest.raises(ValueError, match="not supported on macOS"): CppSimulation._resolve_binary_path("gpu") + @pytest.mark.skipif(sys.platform == "win32", reason="Windows does not honor Unix executable bits") def test_execute_makes_binary_executable(self, tmp_path, monkeypatch): binary = tmp_path / "kspaceFirstOrder-OMP" binary.write_bytes(b"")