Conversation
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 significantly refactors the handling of rupture velocity parameters within the workflow, centralizing them into a new Highlights
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 successfully refactors the rupture velocity parameters into a common configuration, which is a great improvement for consistency and maintainability across hf-sim and realisation-to-srf. The changes are well-implemented throughout the codebase, including configuration files, schemas, and scripts. The refactoring in hf_sim.py to extract helper functions like station_seeds and create_hf_dataset significantly improves code clarity and testability. The addition of new tests, especially using property-based testing with hypothesis, is commendable. I found one minor issue in a new test case, which is detailed in the comments.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors rupture velocity parameters to ensure they remain synchronized between low-frequency (SRF generation) and high-frequency (HF) simulation code paths. It extracts common rupture velocity parameters (rvfrac, rvfrac_shal, rvfrac_deep) from the HFConfig class into a new dedicated RuptureVelocity configuration class that can be shared between both simulation workflows. The PR also includes minor refactoring of the hf_sim module to improve testability by extracting helper functions.
Key changes:
- Created new
RuptureVelocityconfiguration class with shared rupture velocity parameters - Renamed
rvfactorvfracandrvfac_segtorvfrac_segfor naming consistency - Added property-based tests using Hypothesis for
hf_simhelper functions
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| workflow/schemas.py | Added RUPTURE_VELOCITY_SCHEMA and removed rupture velocity fields from HF_CONFIG_SCHEMA; renamed rvfac_seg to rvfrac_seg in SRF_SCHEMA |
| workflow/realisations.py | Added RuptureVelocity dataclass; removed rupture velocity fields from HFConfig; renamed rvfac_seg to rvfrac_seg in SRFConfig |
| workflow/scripts/realisation_to_srf.py | Integrated RuptureVelocity into SRFRealisationContext and _build_genslip_command; passes rupture velocity parameters to genslip |
| workflow/scripts/hf_sim.py | Updated build_hf_input to use RuptureVelocity; extracted station_seeds and create_hf_dataset helper functions; integrated RuptureVelocity in run_hf; uses configured t_sec instead of hardcoded start time |
| workflow/default_parameters/*/defaults.yaml | Moved rupture velocity parameters from hf section to new rupture_velocity section; renamed rvfac_seg to rvfrac_seg |
| workflow/default_parameters/*/root_defaults.yaml | Deleted obsolete root defaults files for all parameter versions |
| tests/test_realisation.py | Added RuptureVelocity to test cases |
| tests/test_hf.py | Added comprehensive tests for build_hf_input, stable_hash, station_seeds, and create_hf_dataset functions using property-based testing with Hypothesis |
| pyproject.toml | Added hypothesis test dependency and updated deptry configuration |
| .github/workflows/deptry.yml | Simplified deptry command to use configuration from pyproject.toml |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…HF input test - Correct typo in comment for output file placeholder - Add assertion for rupture velocity parameters line - Fix typo in comment regarding referential transparency in station seeds test
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 22 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
lispandfound
requested review from
AndrewRidden-Harper,
claudio525,
joelridden and
sungeunbae
lispandfound
dismissed
AndrewRidden-Harper’s stale review
The base branch was changed.
- In genslip, the defaults from the source code are 6.5km +/- 1.5km for shallow transition and are 17.5 +/- 2.5km for deep transition. - The defaults and bounds calculation has been updated for consistency
This PR refactors
hf-simandrealisation-to-srfto have a common set of rupture velocity parameters. The background is explained in slack.Rupture velocity determines the how the wavefront of the rupture moves across the rupture geometry during SRF generation, and HF calculation. It relates to the SRF rupture initiation times, and the lower frequency HF output. In the current workflow settings the rupture velocity is decreased near the surface and at depth. It is consistently adjusted in the high-frequency code path too. However, this is only because Rob applied a consistent set of defaults between the two codebases. When we adjust these parameters we need to make sure they stay in sync. This is the motivation for the PR.
I also slightly refactored the hf-sim code to add some tests. Basically cutting down the main function to have it call more subfunctions and then testing those subfunctions.
The high line count here is just because I have extracted common options into a new defaults file. I did this because I got sick of editing four different files in the same places over and over.