Skip to content

Validate calculate_ims waveform shape and support (nt, 3) input - #244

Merged
lispandfound merged 1 commit into
masterfrom
ai/issue-231
Sep 30, 2026
Merged

lispandfound merged 1 commit into
masterfrom
ai/issue-231

Conversation

@lispandfound

Copy link
Copy Markdown
Contributor

Closes #231

What was wrong

calculate_ims (IM/im_calculation.py) used np.atleast_3d followed by
np.moveaxis(-1, 0) to reshape the input waveform, but the docstring only
said "Waveform data as a NumPy array" and never stated that the only layout
this actually supports is a single station shaped (1, nt, 3):

  • A more intuitive (nt, 3) array was silently reshaped by atleast_3d into
    (nt, 3, 1), which then became (1, nt, 3) after moveaxis — a
    single-component waveform — and failed deep inside IM/ims.py with a
    confusing TypeError: Waveform must have 3 components, but waveform.sizes['component']=1.
  • A multi-station array (n, nt, 3) with n > 1 passed the component check
    but blew up later in _dataset_to_frame with a pandas index length
    mismatch, since that function assumes a single station's worth of rows.

Fix

  • Explicitly validate waveform's shape instead of relying on
    np.atleast_3d: accept (nt, 3) (by adding the leading station axis) or
    (1, nt, 3), and raise a clear ValueError for anything else, including
    multiple stations.
  • Updated the docstring to state the accepted shapes and component order
    (000, 090, ver).

Test

Added two tests to tests/test_ims.py:

  • test_calculate_ims_accepts_2d_waveform: passes both a (1, nt, 3) and the
    equivalent (nt, 3) waveform through calculate_ims and checks the results
    match. Before the fix, this failed with the TypeError about component
    count quoted in the issue.
  • test_calculate_ims_rejects_multiple_stations: passes a (2, nt, 3)
    waveform and checks calculate_ims now raises a clear ValueError. Before
    the fix, this failed with a pandas "Length mismatch" error instead.

Both tests were confirmed to fail against the pre-fix code (for the reasons
above) before the fix was applied, and pass afterwards. .ai/ci-checks was
run in full and passes.

What to check closely

  • The new shape validation only accepts waveform.shape == (1, nt, 3) or
    (nt, 3); if there is a use case elsewhere in the codebase that relies on
    calculate_ims accepting multiple stations at once, that call site would
    now need to loop over stations instead (a search of the repo didn't turn up
    any such caller).

Cost $0.69, 43 turns.

🤖 Generated with Claude Code

Closes #231

## What was wrong

`calculate_ims` (`IM/im_calculation.py`) used `np.atleast_3d` followed by
`np.moveaxis(-1, 0)` to reshape the input `waveform`, but the docstring only
said "Waveform data as a NumPy array" and never stated that the only layout
this actually supports is a single station shaped `(1, nt, 3)`:

- A more intuitive `(nt, 3)` array was silently reshaped by `atleast_3d` into
  `(nt, 3, 1)`, which then became `(1, nt, 3)` after `moveaxis` — a
  single-component waveform — and failed deep inside `IM/ims.py` with a
  confusing `TypeError: Waveform must have 3 components, but
  waveform.sizes['component']=1`.
- A multi-station array `(n, nt, 3)` with `n > 1` passed the component check
  but blew up later in `_dataset_to_frame` with a pandas index length
  mismatch, since that function assumes a single station's worth of rows.

## Fix

- Explicitly validate `waveform`'s shape instead of relying on
  `np.atleast_3d`: accept `(nt, 3)` (by adding the leading station axis) or
  `(1, nt, 3)`, and raise a clear `ValueError` for anything else, including
  multiple stations.
- Updated the docstring to state the accepted shapes and component order
  (000, 090, ver).

## Test

Added two tests to `tests/test_ims.py`:
- `test_calculate_ims_accepts_2d_waveform`: passes both a `(1, nt, 3)` and the
  equivalent `(nt, 3)` waveform through `calculate_ims` and checks the results
  match. Before the fix, this failed with the `TypeError` about component
  count quoted in the issue.
- `test_calculate_ims_rejects_multiple_stations`: passes a `(2, nt, 3)`
  waveform and checks `calculate_ims` now raises a clear `ValueError`. Before
  the fix, this failed with a pandas "Length mismatch" error instead.

Both tests were confirmed to fail against the pre-fix code (for the reasons
above) before the fix was applied, and pass afterwards. `.ai/ci-checks` was
run in full and passes.

## What to check closely

- The new shape validation only accepts `waveform.shape == (1, nt, 3)` or
  `(nt, 3)`; if there is a use case elsewhere in the codebase that relies on
  `calculate_ims` accepting multiple stations at once, that call site would
  now need to loop over stations instead (a search of the repo didn't turn up
  any such caller).

_Cost $0.69, 43 turns._

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@lispandfound lispandfound added ai-generated Opened by the scheduled Claude jobs ai-fix Bot-written fix that needs review labels Sep 29, 2026
@lispandfound

Copy link
Copy Markdown
Contributor Author

@joelridden please look at this closely and check it isn't actually a documentation bug instead of a code issue.

@joelridden

Copy link
Copy Markdown
Contributor

All this is really doing is making sure if we use another function other than waveform_reading.read_ascii (Which already does the reshaped_waveform = waveform_data[np.newaxis, :, :]) to read a waveform and pass it in, to do the same step. Since NZGMDB is using the read_ascii function anyway this is fine to do, I don't consider it that "needed" though, but could be nice to have in future if there is other sources of waveforms to be read and passed into this IM calc function.

@lispandfound
lispandfound merged commit de8599a into master Sep 30, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-fix Bot-written fix that needs review ai-generated Opened by the scheduled Claude jobs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

calculate_ims only accepts a (1, nt, 3) array, but the docstring doesn't say so

2 participants