Clean up README and add macOS hint to C++ backend#692
Conversation
- Remove macOS brew note from README (only needed for C++ backend, not the new NumPy/Python backend) - Update contact email to walter.a.simson@gmail.com - Add macOS dependency hint in executor.py when C++ binary fails: suggests brew install or switching to backend='python' Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
kwave/executor.py
Outdated
| if sys.platform == "darwin": | ||
| print( | ||
| "\nOn macOS, the C++ backend requires additional libraries.\n" | ||
| "Install them with:\n\n" | ||
| " brew install fftw hdf5 zlib libomp\n\n" | ||
| "Alternatively, use backend='python' which requires no extra dependencies.", | ||
| file=sys.stderr, | ||
| ) |
There was a problem hiding this comment.
Hint fires for all macOS errors, not just missing-library failures
The macOS hint is printed whenever any CalledProcessError is raised on macOS — including simulation crashes caused by bad input parameters, file path problems, or other runtime errors. A user whose libraries are correctly installed but whose simulation fails for a different reason would still see "On macOS, the C++ backend requires additional libraries," which is incorrect and potentially confusing.
Consider scoping the hint to cases where the error is more likely to be a missing-dependency failure (e.g. by inspecting e.stderr for well-known dynamic-linker error strings like "Library not loaded" or "image not found"):
if sys.platform == "darwin" and (
"Library not loaded" in (e.stderr or "")
or "image not found" in (e.stderr or "")
):
print(
"\nOn macOS, the C++ backend requires additional libraries.\n"
"Install them with:\n\n"
" brew install fftw hdf5 zlib libomp\n\n"
"Alternatively, use backend='python' which requires no extra dependencies.",
file=sys.stderr,
)If you prefer to keep the hint unconditional, at least soften the wording to something like "If the error above relates to missing libraries…" to avoid misleading users whose errors have a different root cause.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #692 +/- ##
==========================================
- Coverage 74.42% 74.40% -0.02%
==========================================
Files 56 56
Lines 8024 8026 +2
Branches 1569 1570 +1
==========================================
Hits 5972 5972
- Misses 1436 1437 +1
- Partials 616 617 +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:
|
Only show brew install suggestion when stderr contains dyld/linker error strings, not for unrelated simulation failures. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@Greptile-app |
duplicate section
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Overview: no longer says C++ binaries are "required" - Setup: uv sync replaces uv venv + uv pip install - Testing: note that most tests work without MATLAB - Remove: old integration testing / H5 hash sections (C++ pipeline) - Remove: detailed MATLAB reference generation steps (niche workflow) - Remove: hardcoded commit SHA links Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Intro now describes both backends (NumPy/CuPy + C++ binaries) - Remove Colab reference and Binder badge (dead) - Remove transducer walkthrough (too specific for README) - Add uv run example and "no GPU required" note - Simplify mission statement Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- examples_guide.rst: replace legacy directory links with ported .py file links using :ghfile: - first_simulation.rst: simplify "Next Steps" to link to examples_guide instead of individual legacy READMEs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@Greptile-app |
1 similar comment
|
@Greptile-app |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Greptile Summary
This PR cleans up user-facing documentation and adds a targeted macOS dependency hint in the C++ executor error path.
kwave/executor.py: Adds abrew installhint to stderr when aCalledProcessErroroccurs on macOS and the error output contains known dynamic-linker tokens (\"Library not loaded\",\"image not found\", or\"dyld\"). This directly addresses the prior review concern about the hint firing for unrelated failures.docs/README.md: Removes the macOS brew note (now correctly deferred to the C++ backend error path), updates the contact email, and condenses the introduction.docs/examples_guide.rst/docs/get_started/first_simulation.rst: Replaces verbose list-table blocks with lightweight bullet lists; all referenced.pyexample files were confirmed to exist in the repository.docs/development/development_environment.rst: Simplifies setup instructions to auv sync/uv runworkflow while preserving the MATLAB reference-download note.pyproject.toml: Updates Walter Simson's email inauthorsandmaintainersto match the new contact address.Confidence Score: 5/5
Safe to merge — all changes are documentation cleanup and a well-scoped error-path hint with no functional risk
The only code change in executor.py is a narrowly guarded print statement inside an already-failing exception handler; it cannot affect the happy path or introduce new exceptions. The prior review concern about over-broad hinting was addressed by scoping the check to dyld-specific stderr tokens. All referenced documentation file paths were verified to exist in the repository.
No files require special attention
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[run_simulation called] --> B[subprocess.Popen with text=True] B --> C{show_sim_log?} C -- Yes --> D[Stream stdout line-by-line] C -- No --> E[proc.communicate] D --> E E --> F{returncode != 0?} F -- No --> G[parse_executable_output] F -- Yes --> H[raise CalledProcessError with stdout + stderr] H --> I[print stdout / stderr] I --> J{platform == darwin AND stderr contains dyld tokens?} J -- Yes --> K[Print brew install hint + backend='python' suggestion] J -- No --> L[re-raise] K --> LReviews (3): Last reviewed commit: "Update docs to link ported examples, not..." | Re-trigger Greptile