Fix binary_path silently dropped by options_to_kwargs()

[Copilot]

• Extract binary path resolution into CppSimulation._resolve_binary_path(device, binary_path) static method with type annotations
• Add tests for _resolve_binary_path: custom path valid, custom path missing, default OMP, default CUDA, macOS GPU error, non-macOS GPU missing binary
• Add test verifying _execute() sets executable bit on binary
• Improve comment in compat.py explaining why _binary_path is accessed directly
• Move import kwave inside the default-path branch to avoid unnecessary import when custom binary is provided (reviewer feedback)

Greptile Summary

This PR fixes a bug where binary_path set on SimulationExecutionOptions was silently ignored by options_to_kwargs() because the property getter auto-resolved to a default value, making it impossible to distinguish a user-set path from the bundled default. The fix reads _binary_path directly and propagates it through kspaceFirstOrder → CppSimulation.run → _execute.

• compat.py: Reads opts._binary_path (the backing attribute) instead of opts.binary_path (the auto-resolving property), and forwards it as kwargs[\"binary_path\"].
• cpp_simulation.py: Extracts binary-path resolution into _resolve_binary_path() as a testable static method; the import kwave statement is correctly deferred inside the default-path branch so the bundled-binary lookup is skipped when a custom path is supplied.
• tests/: New unit tests cover the resolver's custom-path, default OMP/CUDA, missing-binary, and macOS GPU branches, plus a dedicated test that _execute() sets the executable bit.

Confidence Score: 5/5

Safe to merge — the change is a targeted bug fix with no regressions to existing behaviour.

The fix correctly targets the root cause: the binary_path property on SimulationExecutionOptions auto-resolves to a default, so reading _binary_path directly is the right approach. The import kwave deferral, the static-method extraction, and all new tests are correct. No existing code paths are altered except adding the missing propagation.

No files require special attention.

Important Files Changed

Filename	Overview
kwave/compat.py	Core bug fix: accesses opts._binary_path directly to avoid the auto-resolving property, correctly forwarding user-set binary paths.
kwave/solvers/cpp_simulation.py	Refactors binary resolution into a testable static method with correct lazy import kwave placement; _execute propagates binary_path cleanly.
kwave/kspaceFirstOrder.py	Adds binary_path parameter to the public API and threads it through to CppSimulation.run; ignored for non-cpp backends as documented.
tests/test_compat.py	New test_binary_path test verifies the compat layer correctly forwards _binary_path; uses constructor-level assignment so the non-existent path doesn't need to exist on disk.
tests/test_cpp_simulation.py	Comprehensive new test file covering all resolver branches and the chmod side-effect in _execute; patching approach is correct for static methods.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant compat as options_to_kwargs()
    participant ksFO as kspaceFirstOrder()
    participant run as CppSimulation.run()
    participant exec as CppSimulation._execute()
    participant resolve as _resolve_binary_path()

    Caller->>compat: "SimulationExecutionOptions(binary_path=...)"
    Note over compat: reads opts._binary_path directly
    compat-->>ksFO: "kwargs["binary_path"] = path"

    Caller->>ksFO: "kspaceFirstOrder(..., binary_path=path)"
    ksFO->>run: "cpp_sim.run(..., binary_path=path)"
    run->>exec: "_execute(..., binary_path=path)"
    exec->>resolve: _resolve_binary_path(device, binary_path)

    alt custom binary_path provided
        resolve-->>exec: Path(binary_path) verified to exist
    else binary_path is None
        resolve->>resolve: import kwave
        Note over resolve: select OMP or CUDA binary
        resolve-->>exec: resolved Path verified to exist
    end

    exec->>exec: "resolved.chmod(... | S_IEXEC)"
    exec->>exec: subprocess.run([str(resolved), ...])

Loading

_{Reviews (7): Last reviewed commit: "Merge branch 'master' into copilot/fix-r..." | Re-trigger Greptile}

[codecov]

Codecov Report

❌ Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.04%. Comparing base (9f25de1) to head (0780605).
⚠️ Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
kwave/kspaceFirstOrder.py	0.00%	1 Missing ⚠️
kwave/solvers/cpp_simulation.py	94.44%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #726      +/-   ##
==========================================
+ Coverage   74.82%   75.04%   +0.22%     
==========================================
  Files          56       56              
  Lines        8095     8107      +12     
  Branches     1577     1580       +3     
==========================================
+ Hits         6057     6084      +27     
+ Misses       1422     1404      -18     
- Partials      616      619       +3     

Flag	Coverage Δ
3.10	75.02% <90.47%> (+0.22%)	⬆️
3.11	75.02% <90.47%> (+0.22%)	⬆️
3.12	75.02% <90.47%> (+0.22%)	⬆️
3.13	75.02% <90.47%> (+0.22%)	⬆️
macos-latest	74.97% <90.47%> (+0.22%)	⬆️
ubuntu-latest	74.97% <90.47%> (+0.22%)	⬆️
windows-latest	74.91% <76.19%> (+0.14%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[waltsims]

@greptile-app

[waltsims]

@greptile-apps re-review

/simplify pass applied (3f48987):

• Renamed shadowed binary_path local in _execute() to resolved (the param is str | None; reassigning to a Path silently changed the type).
• Compat.py: collapsed the WHY-comment on _binary_path access to two lines.
• Folded test_custom_path_returns_path_object into test_custom_path_existing_file_is_returned (equality with a Path already proves the return type). Dropped three section-separator and two WHAT-only comments in the test file.

All 23 tests pass; ruff clean.