Add more options for SRF parameters #73
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 enhances the configurability of Synthetic Rupture File (SRF) generation by introducing a comprehensive set of new parameters, many of which are specifically designed for subduction zone source modeling. The changes involve extending the core 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 refactors SRF parameter handling by extracting hardcoded values into configuration and adding new parameters for subduction sources. The changes are consistent across tests, default configurations, and schema definitions. I've found a high-severity bug in how list parameters are formatted for the genslip command, which will likely cause it to fail. I also have two medium-severity suggestions: one to address the removal of the rup_delay parameter which could alter behavior, and another to remove some redundant code for better maintainability. Addressing these points will improve the correctness and quality of the changes.
| f"gwid={params.srf_config.gwid or -1}", | ||
| f"rvfac_seg={params.srf_config.rvfac_seg or -1}", |
There was a problem hiding this comment.
The formatting for gwid and rvfac_seg parameters appears to be incorrect for a command-line tool like genslip. When these lists are not empty, they will be formatted as Python list literals (e.g., gwid=[1.0, 2.0]), which genslip is unlikely to parse correctly. These should probably be comma-separated strings to be parsed correctly by the Fortran program.
| f"gwid={params.srf_config.gwid or -1}", | |
| f"rvfac_seg={params.srf_config.rvfac_seg or -1}", | |
| f"gwid={','.join(map(str, params.srf_config.gwid)) or -1}", | |
| f"rvfac_seg={','.join(map(str, params.srf_config.rvfac_seg)) or -1}", |
| f"gwid={params.srf_config.gwid or -1}", | ||
| f"rvfac_seg={params.srf_config.rvfac_seg or -1}", | ||
| f"seg_delay={int(params.srf_config.seg_delay)}", | ||
| ] |
There was a problem hiding this comment.
The rup_delay=0 parameter was present in the hardcoded genslip command arguments but has been removed in this refactoring. While other parameters were extracted to SRFConfig, this one was dropped. If genslip does not default to 0 for rup_delay, this could introduce a change in behavior. To maintain consistency and avoid potential issues, it would be better to add rup_delay to SRFConfig and the default parameter files, and then add f"rup_delay={params.srf_config.rup_delay}" to this list of arguments.
There was a problem hiding this comment.
Genslip doesn't actually use the rup_delay argument, which is the reason for it being dropped.
There was a problem hiding this comment.
Pull request overview
This PR extracts explicitly set SRF (Standard Rupture Format) parameters from hardcoded values into configurable fields, and adds support for additional parameters used in subduction source simulations. The changes enable more flexible configuration of genslip parameters for seismic rupture modeling.
Key Changes:
- Extracts taper, roughness, and segment parameters (previously hardcoded) into configurable SRF fields
- Adds optional subduction-specific parameters (ymag_exp, xmag_exp, kx_corner, ky_corner)
- Refactors SRFConfig.to_dict() to use dataclasses.asdict() for more maintainable serialization
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| workflow/scripts/realisation_to_srf.py | Updates genslip command construction to use configurable parameters instead of hardcoded values; adds dynamic parameter extension for subduction settings |
| workflow/schemas.py | Adds validation schemas for new SRF parameters with appropriate type constraints; moves comment block for better organization |
| workflow/realisations.py | Extends SRFConfig dataclass with new parameter fields; simplifies to_dict() method using dataclasses.asdict() |
| workflow/default_parameters/*/defaults.yaml | Adds default values for new SRF parameters across all configuration versions |
| tests/test_realisation.py | Updates test expectations to include new SRF parameters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| f"gwid={params.srf_config.gwid or -1}", | ||
| f"rvfac_seg={params.srf_config.rvfac_seg or -1}", |
There was a problem hiding this comment.
The or operator with list types will not work as intended. When gwid or rvfac_seg are empty lists (the default []), they evaluate as falsy and -1 is used correctly. However, when they contain values, the entire list object will be used in string formatting, resulting in output like gwid=[1.0, 2.0] instead of a proper format. If these parameters accept lists, they should be formatted similarly to rayset in hf_sim.py (e.g., ' '.join(str(x) for x in gwid) or '-1'). Alternatively, if they should be single values, the schema and type annotations should be changed from list[float] to float | None.
| f"gwid={params.srf_config.gwid or -1}", | |
| f"rvfac_seg={params.srf_config.rvfac_seg or -1}", | |
| f"gwid={' '.join(str(x) for x in params.srf_config.gwid) if params.srf_config.gwid else -1}", | |
| f"rvfac_seg={' '.join(str(x) for x in params.srf_config.rvfac_seg) if params.srf_config.rvfac_seg else -1}", |
| gwid: list[float] | ||
| rvfac_seg: list[float] | ||
| seg_delay: bool |
There was a problem hiding this comment.
These new fields lack docstrings. Consider adding documentation to describe what each parameter controls, following the pattern established for other fields in this class (e.g., lines 360, 363, 365, 368). This is especially important for parameters like gwid, rvfac_seg, and seg_delay whose purpose may not be immediately clear to users.
| gwid: list[float] | |
| rvfac_seg: list[float] | |
| seg_delay: bool | |
| gwid: list[float] | |
| """List of grid widths for each segment, used to define the segmentation of the fault.""" | |
| rvfac_seg: list[float] | |
| """List of rupture velocity factors for each segment, controlling the speed of rupture propagation per segment.""" | |
| seg_delay: bool | |
| """If True, applies a delay between segments during rupture propagation.""" |
| f"alpha_rough={params.srf_config.alpha_rough}", | ||
| f"gwid={params.srf_config.gwid or -1}", | ||
| f"rvfac_seg={params.srf_config.rvfac_seg or -1}", | ||
| f"seg_delay={int(params.srf_config.seg_delay)}", |
There was a problem hiding this comment.
[nitpick] The seg_delay parameter is a boolean converted to int (0 or 1). For clarity and to match the pattern used elsewhere in the codebase, consider adding a descriptive comment or using a more explicit conversion like 1 if params.srf_config.seg_delay else 0 to make the boolean-to-integer mapping clear.
| f"seg_delay={int(params.srf_config.seg_delay)}", | |
| # Explicitly convert boolean to int for clarity (True -> 1, False -> 0) | |
| f"seg_delay={1 if params.srf_config.seg_delay else 0}", |
This PR extracts explicitly set SRF parameters, and adds some extra parameters used for subduction sources.