Update IM calc to use NZGMDB IM settings#89
Conversation
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…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
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- 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
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 default simulation parameters and intensity measure calculations to align with NZGMDB standards. It introduces a more robust and modular system for managing configuration defaults, particularly for rupture velocity and IM periods, and enhances the high-frequency simulation workflow with new utility functions and comprehensive testing. The changes aim to improve the accuracy and maintainability of seismic simulation settings. Highlights
Changelog
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 introduces a significant and beneficial refactoring of the default parameter handling, creating a hierarchical structure with a new root defaults file. This greatly improves maintainability by reducing duplication. It also refactors rupture velocity parameters into a new RuptureVelocity configuration class, improving code organization and consistency by renaming rvfac to rvfrac. As described, the intensity measure calculation parameters have been updated to match NZGMDB standards, including the addition of PGD. New tests have been added to cover these changes. I've found one potential issue regarding the handling of empty YAML files which could lead to an error.
There was a problem hiding this comment.
Pull request overview
This pull request refactors rupture velocity configuration and updates intensity measure calculation defaults to align with NZGMDB standards. It builds on PR #80's rupture velocity refactor by extracting common default parameters into a centralized root configuration file and updating IM calculation periods.
Changes:
- Extracts rupture velocity parameters (rvfrac, rvfrac_shal, rvfrac_deep, and depth configurations) into a new
RuptureVelocityclass, removing them fromHFConfig - Consolidates common default parameters into a new
workflow/default_parameters/root/defaults.yamlfile with a deep-merge loading strategy - Updates intensity measure calculation to include PGD and use NZGMDB-aligned pSA periods (expanded from 31 to 107 periods)
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| workflow/scripts/realisation_to_srf.py | Adds RuptureVelocity parameter to SRF generation, passes rupture velocity config to genslip command |
| workflow/scripts/hf_sim.py | Refactors to use RuptureVelocity config, extracts helper functions (rupture_velocity_hf_transition_bands, station_seeds, create_hf_dataset) for better testability |
| workflow/schemas.py | Adds RUPTURE_VELOCITY_SCHEMA, removes rupture velocity fields from HF_CONFIG_SCHEMA, renames rvfac_seg to rvfrac_seg |
| workflow/realisations.py | Adds RuptureVelocity class, removes rupture velocity fields from HFConfig, renames rvfac_seg to rvfrac_seg in SRFConfig |
| workflow/defaults.py | Implements _merge_defaults function for deep-merging root and version-specific defaults |
| workflow/default_parameters/root/defaults.yaml | New centralized defaults file with common configuration (emod3d, hf, rupture_velocity, srf, velocity_model, velocity_model_1d, im, hf_velocity_model_1d) |
| workflow/default_parameters/v*/root_defaults.yaml | All removed (migrated to root/defaults.yaml) |
| workflow/default_parameters/v*/defaults.yaml | Stripped down to only version-specific overrides (resolution, bb parameters) |
| workflow/default_parameters/develop/defaults.yaml | Updated with new rupture_velocity section and rvfrac_seg rename |
| workflow/default_parameters/README.md | Deleted (outdated version documentation) |
| tests/test_realisation_to_srf.py | New test file for genslip command building with rupture velocity parameters |
| tests/test_realisation.py | Adds RuptureVelocity to test_defaults_are_loadable, updates rvfac_seg to rvfrac_seg |
| tests/test_hf.py | New comprehensive test file for HF simulation functions (build_hf_input, stable_hash, station_seeds, create_hf_dataset) |
| pyproject.toml | Adds "types" to pep621_dev_dependency_groups |
| .github/workflows/deptry.yml | Simplifies deptry command (removed explicit dependency group flags) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Qp: 460.00 | ||
| Qs: 230.00 | ||
| im: | ||
| ims: ["PGA", "PGV", "PGD", "CAV", "AI", "Ds575", "Ds595", "pSA", "FAS"] |
There was a problem hiding this comment.
PGD has been added to the default intensity measures list, but there is no handler for it in the im_function_map in workflow/scripts/im_calc.py (which only has handlers for PGA, PGV, CAV, AI, Ds575, Ds595, pSA, and FAS). This will cause intensity measure calculation to fail when PGD is requested. Either remove PGD from the default list or add the corresponding handler to im_calc.py.
| ims: ["PGA", "PGV", "PGD", "CAV", "AI", "Ds575", "Ds595", "pSA", "FAS"] | |
| ims: ["PGA", "PGV", "CAV", "AI", "Ds575", "Ds595", "pSA", "FAS"] |
|
Tests will fail until #88 is merged. |
Note
Merge #80 and #88 first!