Repository navigation
Conversation
Test functions that ran the same code with different inputs and expected results are now one test with `pytest.mark.parametrize`, in 15 of the 22 test files. Each case keeps its own test ID (named after the original test or its comment), so the same cases still run and are reported separately: 108 test functions become 40 parametrized tests with 109 cases (the old invalid-unit-names test in test_class_ground_motion.py had two checks, which are now two cases). - Where the cases call different methods of the same kind (e.g., the four ground-motion filters, `get_HH_x_param()`/`get_H4_x_param()`, or `get_f0_BH()`/`get_f0_RO()`), the unbound method is the parameter - Boolean parameters are keyword-only (FBT001), as in CONTRIBUTING.md - The expected-failure cases that matched against an empty string (`match=''`, which pytest warns always passes) now use no `match` - The HH case of the multi-layer list-operations test now checks `n_layer` after deleting a layer, like the MKZ case did, instead of checking `len()` twice Tests with different assertions or setup are left as they are, and so are the sequential checks inside one test function. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C1a5vwxyeL4atk9tYUUEoQ
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C1a5vwxyeL4atk9tYUUEoQ
- Name the combined HH/H4 and HH/MKZ tests after both models, e.g., `test_HH_x_or_H4_x_fit_single_layer` and `test_HH_MKZ_param_multi_layer__can_initiate_object_from_file`, and `test_f0` as `test_f0_BH_or_f0_RO` - Name the cases of `test_construct_curves`, `test_param_serialize`, and `test_serialize_to_2D_array` after the original test functions (e.g., `from_HH_G_parameters`) - Move a comment in test_helper_hh_model.py into the `pytest.param()` that it describes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C1a5vwxyeL4atk9tYUUEoQ
- Leave out optional arguments in the cases that used to leave them out, so that their defaults are tested again: `dt` in the ground motion loading tests, and `OCR_upper_limit` in `test_calc_OCR`. The cases now pass a `kwargs` dict, like `test_class_svm.py` does - Pass callables (`lambda gm: gm.pga_in_g`, and lambdas that call `get_curve_matrix()` with the keyword argument) instead of attribute and keyword names as strings - Combine the differential-evolution and DEAP multi-layer fitting tests into `test_HH_x_or_H4_x_fit_multi_layer`. The DEAP cases still pass when DEAP is not installed, as before; an `ImportError` in the differential-evolution cases is no longer swallowed - Share one `multi_layer_case` fixture among the 4 HH/MKZ multi-layer parameter tests, instead of repeating the case list. The list operations test now deletes layer 3 in both cases (it was 6 for MKZ) - Rename `test_param_multi_layer__test_list_operations` to `test_HH_MKZ_param_multi_layer__list_operations`, and `test_calc_damping_from_stress_strain__case_3` to `..._initial_damping_almost_0`, after what it tests (its siblings' IDs are no longer numbered) - Pass a copy, not a view, of the module-level `DATA_5_COLUMNS` array in the `incorrect_number_of_columns` case Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016MPgzyHE9jy5vP7heBxZhA
- `_get_freq_interval()` built its frequency array as `np.linspace(df, fmax, num=half_n)`, so element k was (k + 1) * df, but `amplify_motion()` uses element k as the frequency of FFT bin k (bin 0 is 0 Hz). The transfer function was applied one bin too high. It now uses `np.arange(half_n) * df` (#78) - `fmax` is now the last element of that array (the highest FFT bin, i.e., the Nyquist frequency for an even length), instead of one bin above it. So with `extrap_tf=False`, a transfer function that reaches the Nyquist frequency no longer makes `amplify_motion()` downsample the motion - The amplification and phase plots of `amplify_motion()` skip 0 Hz, which a logarithmic axis cannot show; update the comments on the 0 Hz value - Add tests: a cosine at an FFT bin through a frequency-dependent transfer function (amplify and deconvolve), a transfer function up to the Nyquist frequency without `extrap_tf`, and the frequency array itself - `sim.linear()` and `linear_site_resp()` now agree to a correlation of 0.99999 (elastic) and 0.99986 (rigid), so raise the thresholds of the tests that compare them to 0.999 and fix the comment that blamed `sim.linear()` for the difference Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7fK8T2pPLf3tPCnMCKL1T
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7fK8T2pPLf3tPCnMCKL1T
#74 was squash-merged into main, so main's files are identical to the branch's old base. Every conflict is resolved with this branch's version. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7fK8T2pPLf3tPCnMCKL1T
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7fK8T2pPLf3tPCnMCKL1T
Co-authored-by: jsh9 <jsh9@users.noreply.github.com>
Co-authored-by: jsh9 <jsh9@users.noreply.github.com>
Co-authored-by: jsh9 <jsh9@users.noreply.github.com>
Co-authored-by: jsh9 <jsh9@users.noreply.github.com>
This was referenced Oct 11, 2026
This branch has not been deployed
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
3 participants
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.
Fixes #78. One of the four P0 fixes from #114.
Problem
_get_freq_interval()built its frequency array asnp.linspace(df, fmax, num=half_n), so element k was (k + 1)·df.amplify_motion()uses element k as the frequency of FFT bin k, where bin 0 is 0 Hz. So the transfer function was applied one bin too high.linear_site_resp(),Ground_Motion.amplify(),deconvolve(),amplify_by_tf()andSite_Effect_Adjustment.run().linear_site_resp()had correlation 0.99791 (elastic) and 0.96969 (rigid). With this fix it is 1.00000 and 0.99992.Changes
All in
PySeismoSoil/helper_site_response.py:_get_freq_interval():f_array = np.arange(half_n) * df, so element k is the frequency of FFT bin k. This is the fix proposed in the issue.fmax(beyond the issue): it is now(half_n - 1) * df, the last element off_array(the Nyquist frequency when n is even). Before, it was one bin above that.extrap_tf=False,amplify_motion()halves the input until the transfer function reachesfmax. With the old value, a transfer function covering exactly 0–50 Hz for dt = 0.01 s still halved the output's sampling rate, although the docstring says that is enough.extrap_tf=True, the extrapolated point now falls exactly on the last bin.tf_ss[0] = np.real(tf_ss[0])is still right. The comments there and "Note (1)" are updated to describe hownp.interp()fills 0 Hz.show_figplot: it now skips the 0 Hz point, which the logarithmic frequency axes cannot show.CHANGELOG.md: a "Fixed" entry.Tests
11 new or changed tests, all of which fail on the old code:
tests/test_helper_site_response.py:test_amplify_motion__cosine_at_an_FFT_bin[amplify|deconvolve × even|odd length]: a cosine at FFT bin 10 through |TF| = 1 + f (zero phase) comes out multiplied (or divided) by |TF| at its frequency. Old code: 2.1 instead of 2.0.test_amplify_motion__tf_up_to_the_Nyquist_frequency[even|odd]: withextrap_tf=False, a 0–50 Hz transfer function causes no downsampling. Old code:(500, 2) == (1000, 2)fails.test_get_freq_interval[even|odd]:f_arrayequalsnp.fft.rfftfreq(n, dt), andfmaxis its last value.tests/test_helper_simulations.py::test_linear[elastic|rigid]:sim.linear()for the difference is corrected.tests/test_class_simulation.py::test_linear: the threshold goes from 0.99 to 0.999 (0.998177 before, 0.999991 now).No benchmark values changed. The deconvolution round-trip tests passed before and still pass, because the shift cancels out there.
Testing
These were run locally on Python 3.13, after merging
main(which now includes #74):python -m pytest tests: 249 passed.pre-commit run -aandpydoclint PySeismoSoil: pass.tox -e run-notebooks: all 14 notebooks ran without errors.Notes for review
Two related off-by-one problems were found while fixing this, and are not changed here:
helper_signal_processing.fourier_transform()labels FFT bin 0 (0 Hz) asdf(freq_array = np.arange(1, ...) / (N * dt)), so every frequency label is one bin too high. It feedsGround_Motion.get_Fourier_spectrum(),calc_transfer_function(),compare_two_accel()and the GoF scores.helper_simulations.linear()evaluates bin k at k·df + df/15, which is why it reaches only 0.99999 rather than exactly 1 against the exact solution.The other P0 fixes (#77 in #117; #79 in #116; #60 in #115) are in separate PRs into
mainand change different library code. All of them re-run the notebooks and add a "Fixed" group toCHANGELOG.md, so whichever merges second will conflict there. To resolve: keep both entries and re-run the notebooks.🤖 Generated with Claude Code
https://claude.ai/code/session_01W7fK8T2pPLf3tPCnMCKL1T