Skip to content

Clean up README and add macOS hint to C++ backend#692

Merged
waltsims merged 8 commits intomasterfrom
cleanup-readme-macos
Mar 29, 2026
Merged

Clean up README and add macOS hint to C++ backend#692
waltsims merged 8 commits intomasterfrom
cleanup-readme-macos

Conversation

@waltsims
Copy link
Copy Markdown
Owner

@waltsims waltsims commented Mar 29, 2026

  • 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'

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 a brew install hint to stderr when a CalledProcessError occurs 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 .py example files were confirmed to exist in the repository.
  • docs/development/development_environment.rst: Simplifies setup instructions to a uv sync / uv run workflow while preserving the MATLAB reference-download note.
  • pyproject.toml: Updates Walter Simson's email in authors and maintainers to 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

Filename Overview
kwave/executor.py Adds a macOS-scoped hint when C++ binary fails with dynamic linker errors; properly guarded behind platform and stderr content checks
docs/README.md Removes macOS brew note (C++ backend-only concern), updates contact email, and condenses introduction text
docs/development/development_environment.rst Condensed setup guide; preserves MATLAB reference download note; switches to uv sync/uv run idioms
docs/examples_guide.rst Replaces verbose list-tables with lightweight bullet lists; all referenced example .py files verified to exist; uses ghfile extlink role with explicit titles (supported by Sphinx 5+)
docs/get_started/first_simulation.rst Simplifies Next Steps section to a single cross-reference instead of a categorised example list
pyproject.toml Updates Walter Simson's email in authors and maintainers to gmail address, consistent with README and other docs changes

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 --> L
Loading

Reviews (3): Last reviewed commit: "Update docs to link ported examples, not..." | Re-trigger Greptile

- 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>
Comment on lines +57 to +64
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,
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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
Copy link
Copy Markdown

codecov bot commented Mar 29, 2026

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.40%. Comparing base (5d50748) to head (1b76143).
⚠️ Report is 9 commits behind head on master.

Files with missing lines Patch % Lines
kwave/executor.py 0.00% 1 Missing and 1 partial ⚠️
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     
Flag Coverage Δ
3.10 74.40% <0.00%> (-0.02%) ⬇️
3.11 74.40% <0.00%> (-0.02%) ⬇️
3.12 74.40% <0.00%> (-0.02%) ⬇️
3.13 74.40% <0.00%> (-0.02%) ⬇️
macos-latest 74.38% <0.00%> (-0.02%) ⬇️
ubuntu-latest 74.38% <0.00%> (-0.02%) ⬇️
windows-latest 74.39% <0.00%> (-0.02%) ⬇️

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.

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>
@waltsims
Copy link
Copy Markdown
Owner Author

@Greptile-app

waltsims and others added 5 commits March 29, 2026 12:08
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>
@waltsims
Copy link
Copy Markdown
Owner Author

@Greptile-app

1 similar comment
@waltsims
Copy link
Copy Markdown
Owner Author

@Greptile-app

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@waltsims waltsims merged commit 3a7ae8f into master Mar 29, 2026
91 checks passed
@waltsims waltsims deleted the cleanup-readme-macos branch March 29, 2026 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant