Skip to content

Conversation

@claudio525
Copy link

Adds support for generating custom site grids. See the Wiki page for details.

@gemini-code-assist
Copy link

Summary of Changes

Hello @claudio525, 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 workflow by introducing a robust framework for generating custom virtual simulation site grids. This new capability allows users to define specific criteria for site selection, ensuring consistency across different simulation sets while maintaining the flexibility for tailored analyses. The change also involves the removal of an outdated 'merge timeslices' utility, simplifying the overall system architecture.

Highlights

  • New Site Generation System: Introduced a comprehensive system for generating custom virtual site grids, allowing for flexible sub-sampling of a high-density general grid based on various filters like region, uniform spacing, land-only, and basin-specific spacing.
  • Removal of Merge Timeslices Functionality: The merge-ts utility, its associated Python script, Cython loop, and related configurations have been completely removed from the workflow, streamlining the codebase.
  • Enhanced Documentation: New detailed documentation has been added to the wiki, outlining the background, custom grid generation process, and plot generation capabilities of the new site generation feature.
  • New Command-Line Interface: A new command-line script, site_gen_cmds.py, has been added to facilitate the generation of custom site grids and their visual representation.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds comprehensive support for generating custom virtual site grids for earthquake simulations. The main purpose is to enable creation of domain-specific site grids by sub-sampling a high-density "general" grid, ensuring consistency across simulation sets while allowing customization. Additionally, this PR removes the deprecated merge timeslices functionality.

Key changes:

  • New site_gen.py module implementing custom grid generation with configurable filters (land-only, regional bounds, uniform spacing, basin-specific spacing)
  • CLI commands in site_gen_cmds.py for grid generation and visualization
  • Removal of merge timeslices stage and related code

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
workflow/site_gen.py Core implementation of general and custom grid classes with filtering capabilities
workflow/scripts/site_gen_cmds.py Command-line interface for grid generation and plotting
workflow/scripts/plan_workflow.py Removed deprecated MergeTimeslices stage identifier
workflow/scripts/merge_ts_loop.pyx Deleted Cython implementation for timeslice merging
workflow/scripts/merge_ts.py Deleted merge timeslices script
workflow/init.py Removed merge timeslices documentation reference
wiki/Stages.md Removed merge timeslices stage documentation
wiki/Site-Generation.md Added comprehensive documentation for site generation feature
setup.py Deleted setup file for Cython extensions
pyproject.toml Removed merge-ts console script entry point

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a significant new feature for generating custom site grids, while also cleaning up by removing the old merge-ts functionality. The new site generation code is well-structured, but I've found several critical issues related to coordinate transformations that will cause incorrect geographical plotting of domains and sources. There are also some high-severity correctness issues in the grid-masking logic and inconsistencies between the implementation and the new documentation. Addressing these points will be crucial for the feature to work as intended and be understood by users. I've also included some medium-severity suggestions to improve code quality and usability.

@lispandfound
Copy link
Contributor

These LLM comments look mostly reasonable. I'll take a pass at it once they are resolved.

@lispandfound
Copy link
Contributor

See also: the pytest coverage checks are failing. You need some tests for your code.

@claudio525 claudio525 marked this pull request as draft October 23, 2025 20:35
@claudio525 claudio525 requested a review from Copilot October 28, 2025 01:46
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@claudio525 claudio525 requested a review from Copilot November 4, 2025 00:31
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- basin: The basin the site is in (if any).
- Z1.0: The Z1.0 value for the site (if vel_model_version is provided).
- Z2.5: The Z2.5 value for the site (if vel_model_version is provided).
- site_code: The site code.
Copy link

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring is missing documentation for the 'Vs30' column. According to line 701, 'Vs30' is loaded from the NZGMDB data for real sites, and this column is referenced in the plotting code (site_gen_cmds.py lines 169, 177, 199, 207). The documentation should include: - Vs30: Time-averaged shear-wave velocity in the top 30 meters (m/s) (for real sites only).

Suggested change
- site_code: The site code.
- site_code: The site code.
- Vs30: Time-averaged shear-wave velocity in the top 30 meters (m/s) (for real sites only).

Copilot uses AI. Check for mistakes.
Comment on lines +415 to +419





Copy link

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove unnecessary blank lines (lines 415-418). These are leftover whitespace that should be cleaned up.

Suggested change

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants