Validate calculate_ims waveform shape and support (nt, 3) input - #244
Merged
Merged
Conversation
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>
Contributor
Author
|
@joelridden please look at this closely and check it isn't actually a documentation bug instead of a code issue. |
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. |
joelridden
approved these changes
Sep 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #231
What was wrong
calculate_ims(IM/im_calculation.py) usednp.atleast_3dfollowed bynp.moveaxis(-1, 0)to reshape the inputwaveform, but the docstring onlysaid "Waveform data as a NumPy array" and never stated that the only layout
this actually supports is a single station shaped
(1, nt, 3):(nt, 3)array was silently reshaped byatleast_3dinto(nt, 3, 1), which then became(1, nt, 3)aftermoveaxis— asingle-component waveform — and failed deep inside
IM/ims.pywith aconfusing
TypeError: Waveform must have 3 components, but waveform.sizes['component']=1.(n, nt, 3)withn > 1passed the component checkbut blew up later in
_dataset_to_framewith a pandas index lengthmismatch, since that function assumes a single station's worth of rows.
Fix
waveform's shape instead of relying onnp.atleast_3d: accept(nt, 3)(by adding the leading station axis) or(1, nt, 3), and raise a clearValueErrorfor anything else, includingmultiple stations.
(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 theequivalent
(nt, 3)waveform throughcalculate_imsand checks the resultsmatch. Before the fix, this failed with the
TypeErrorabout componentcount quoted in the issue.
test_calculate_ims_rejects_multiple_stations: passes a(2, nt, 3)waveform and checks
calculate_imsnow raises a clearValueError. Beforethe 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-checkswasrun in full and passes.
What to check closely
waveform.shape == (1, nt, 3)or(nt, 3); if there is a use case elsewhere in the codebase that relies oncalculate_imsaccepting multiple stations at once, that call site wouldnow 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