Skip to content

fix: fix t_array setter ValueError when passing numpy array#732

Merged
waltsims merged 1 commit into
waltsims:masterfrom
wangwangbobo:fix/t_array-setter
May 16, 2026
Merged

fix: fix t_array setter ValueError when passing numpy array#732
waltsims merged 1 commit into
waltsims:masterfrom
wangwangbobo:fix/t_array-setter

Conversation

@wangwangbobo
Copy link
Copy Markdown
Contributor

@wangwangbobo wangwangbobo commented May 16, 2026

Fixes the t_array setter which raises ValueError: The truth value of an array with more than one element is ambiguous when trying to set a numpy array.

The issue was that t_array == 'auto' returns a boolean array when t_array is a numpy array, causing the if statement to fail.

Fixed by using isinstance(t_array, str) and t_array == 'auto' instead.

Greptile Summary

This PR fixes a ValueError: The truth value of an array with more than one element is ambiguous crash in the t_array setter by guarding the == "auto" comparison with an isinstance(t_array, str) check, which short-circuits before NumPy can produce a boolean array.

  • The one-line change in kwave/kgrid.py replaces if t_array == "auto": with if isinstance(t_array, str) and t_array == "auto":, correctly handling both string ("auto") and numpy array inputs.
  • The existing self.Nt == "auto" / self.dt == "auto" comparisons in the getter (line 100) remain unaffected because those attributes are always plain strings or scalars, not numpy arrays.

Confidence Score: 5/5

Safe to merge — the change is a minimal, well-targeted guard that fixes a crash without altering any other behavior.

The fix is a single-line change that correctly handles both the existing 'auto' string path and the previously broken numpy array path. The isinstance short-circuit is the idiomatic Python solution, the rest of the setter is untouched, and the getter's own 'auto' comparisons operate on plain scalar attributes so they are unaffected.

No files require special attention.

Important Files Changed

Filename Overview
kwave/kgrid.py Single-line fix to t_array setter: guards the 'auto' string comparison with an isinstance check to prevent ValueError when a numpy array is passed.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant t_array setter
    participant kWaveGrid

    Caller->>t_array setter: set t_array = numpy_array
    Note over t_array setter: isinstance(t_array, str) → False<br/>short-circuit: skip "auto" branch
    t_array setter->>t_array setter: extract Nt_temp, dt_temp
    t_array setter->>t_array setter: validate (zero start, even spacing, increasing)
    t_array setter->>kWaveGrid: self.Nt = Nt_temp, self.dt = dt_temp

    Caller->>t_array setter: set t_array = "auto"
    Note over t_array setter: isinstance(t_array, str) → True<br/>t_array == "auto" → True
    t_array setter->>kWaveGrid: self.Nt = "auto", self.dt = "auto"
Loading

Reviews (1): Last reviewed commit: "fix: fix t_array setter ValueError when ..." | Re-trigger Greptile

The setter compared t_array == 'auto' which raises ValueError when
t_array is a numpy array. Use isinstance() check instead.
@codecov
Copy link
Copy Markdown

codecov Bot commented May 16, 2026

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 74.82%. Comparing base (9f25de1) to head (5dcaad6).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
kwave/kgrid.py 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #732   +/-   ##
=======================================
  Coverage   74.82%   74.82%           
=======================================
  Files          56       56           
  Lines        8095     8095           
  Branches     1577     1577           
=======================================
  Hits         6057     6057           
  Misses       1422     1422           
  Partials      616      616           
Flag Coverage Δ
3.10 74.79% <0.00%> (ø)
3.11 74.79% <0.00%> (ø)
3.12 74.79% <0.00%> (ø)
3.13 74.79% <0.00%> (ø)
macos-latest 74.74% <0.00%> (ø)
ubuntu-latest 74.74% <0.00%> (ø)
windows-latest 74.76% <0.00%> (ø)

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

Hi @wangwangbobo,

Thanks for the PR. This looks good. Can you add a short test case for the fix please?

Best,
Walter

Copy link
Copy Markdown
Owner

@waltsims waltsims left a comment

Choose a reason for hiding this comment

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

LGTM. The isinstance guard correctly short-circuits the numpy ambiguous-truth-value crash, the existing 'auto' string path is preserved, and the rest of the setter is untouched. Greptile 5/5, 0 unresolved.

@waltsims waltsims merged commit cf4ec8f into waltsims:master May 16, 2026
17 of 18 checks passed
This was referenced May 16, 2026
@wangwangbobo
Copy link
Copy Markdown
Contributor Author

Thanks for the quick merge, Walter! 🙏

The test case is a good idea — I'll add one in a follow-up. The fix is straightforward (swapping for ), so a simple test passing a numpy array to the setter should cover it. Will submit a PR with the test shortly. 👍

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.

2 participants