Conversation
- pSA now calculated with either linear or constant newmark according to stability. - rotd calculation in parallel
- use `select_nth_unstable` instead of sorting to reduce post-processing. - use branchless absmax to speed up rotd fold
- clippy + cargo test
Summary of ChangesHello @lispandfound, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a substantial upgrade to the pseudo-spectral acceleration (pSA) calculation pipeline by integrating a new, highly optimized Rust solver. The primary goal is to overcome previous Python memory and CPU bottlenecks, enabling much faster processing of large-scale seismic simulations. Beyond speed, the changes enhance the numerical accuracy and stability of pSA results through the adoption of 64-bit floating-point precision and sophisticated refinements to the Newmark-Beta method, including a new adaptive approach for selecting the most appropriate acceleration approximation. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great initiative to improve performance by migrating the pSA solver to a Rust extension. The new Rust code is well-structured, thoroughly tested, and includes thoughtful improvements to the numerical method. The Python integration is also mostly correct. However, I've identified a couple of critical bugs in the Python glue code that will lead to incorrect calculations. I've also included some suggestions to improve robustness and clarity in both the application and test code.
tests/test_ims.py
Outdated
| data[im].values, abs=5e-4, rel=0.01, nan_ok=True | ||
| ), ( | ||
| f"Results for {im} do not match!\n{result}" | ||
| ) # 5e-6 implies rounding to five decimal places | ||
|
|
There was a problem hiding this comment.
The comment on line 381 is inconsistent with the tolerance used in the assertion. The comment mentions 5e-6 for five decimal places, but the pytest.approx call uses abs=5e-4 and rel=0.01. This discrepancy can be misleading for anyone maintaining these tests in the future. Please update the comment to reflect the actual tolerances being used.
) # Tolerances are looser due to solver changes
tests/test_ims.py
Outdated
| data[im].values, abs=5e-4, rel=0.01, nan_ok=True | ||
| ), ( | ||
| f"Results for {im} do not match!\n{result}" | ||
| ) # 5e-6 implies rounding to five decimal places | ||
|
|
There was a problem hiding this comment.
The comment on line 409 regarding the tolerance (5e-6) does not match the actual values used in the pytest.approx assertion (abs=5e-4, rel=0.01). To avoid confusion, please update the comment to align with the code.
) # Tolerances are looser due to solver changes
|
/gemini review |
|
Note The number of changes in this pull request is too large for Gemini Code Assist to generate a review. |
This reverts commit c9918c2.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 43 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,3 +1,5 @@ | |||
| """ASCII Waveform reading code.""" | |||
There was a problem hiding this comment.
The comment on line 1 appears to be a module docstring. In Python, module-level docstrings should be the first statement in the file (before any imports), not a comment. This should be changed from a comment to a proper docstring using triple quotes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
lispandfound
requested review from
AndrewRidden-Harper,
claudio525 and
sungeunbae
|
@joelridden I just need fresh benchmark files to update the tests to pass. |
This PR introduces a new highly accelerated rust pSA + rotd solver which computes the pSA results in a few minutes of tens of thousands of stations and hundreds of thousands of timesteps on a 10 year desktop. I am hoping this is going to be fast enough to redo the alpine simulations pSA, after the mysterious slowdown that occurred on RCH recently turned out to be related to Python memory and CPU bottlenecks. There are few technical improvements to the quality of the pSA results besides the speed:
Obviously, steps 2 and 3 required really careful work on the newmark beta solver. Numerical solvers are notoriously dangerous guns to play with due to the complex equations that go into them.
Note
High line count is due to more CSV benchmark tests, and some lock files for CI reproducibility.