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 modernizes the project's dependency management by migrating from Highlights
Changelog
Ignored Files
Activity
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 good improvement, migrating dependency management from requirements.txt to pyproject.toml and refactoring the realisation plotting logic. The code changes for the plotting fix look correct and align with the goal of using rrup boundary polygons. I've left a few comments regarding the dependency migration, suggesting to add a missing test dependency and to sort the lists for better maintainability. I also found an issue in visualisation/realisation.py where a docstring example was not updated after a function signature refactoring, which would cause an error. Overall, great work on modernizing the project setup and fixing the plotting logic.
| def plot_realisation( | ||
| realisation_ffp: Path, | ||
| magnitudes: Magnitudes, | ||
| domain_parameters: DomainParameters, | ||
| velocity_model_parameters: VelocityModelParameters, | ||
| source_config: SourceConfig, | ||
| latitude_pad: float = 0, | ||
| longitude_pad: float = 0, | ||
| title: str | None = None, | ||
| subtitle: str | None = None, | ||
| width: float = 10, | ||
| show_geonet_stations: bool = False, | ||
| show_geometry: bool = True, | ||
| show_pgv_targets: bool = False, | ||
| pgv_targets: list[float] | None = None, | ||
| show_rrup_targets: bool = False, | ||
| stations: Path | None = None, | ||
| ) -> pygmt.Figure: |
There was a problem hiding this comment.
The function signature has been refactored, which is a great improvement. However, the corresponding docstring example (starting on line 261) has not been fully updated. It still shows a call with realisation_ffp, which is no longer a parameter, and will cause a TypeError. Please update the example to be consistent with the new function signature.
| dependencies = [ | ||
| "cartopy", | ||
| "matplotlib", | ||
| "numpy", | ||
| "ffmpeg-python", | ||
| "pygmt", | ||
| "pygmt-helper>=2025.12.1", | ||
| "source-modelling>=2025.12.1", | ||
| "workflow @ git+https://github.com/ucgmsim/workflow.git@pegasus", | ||
| "qcore-utils>=2025.12.2", | ||
| "geopandas", | ||
| "pandas", | ||
| "shapely", | ||
| "scipy >= 1.15.0", | ||
| "pooch", | ||
| "typer", | ||
| "tqdm", | ||
| ] |
There was a problem hiding this comment.
For better maintainability and to avoid potential merge conflicts, it's a good practice to keep the main dependency list sorted alphabetically.
| dependencies = [ | |
| "cartopy", | |
| "matplotlib", | |
| "numpy", | |
| "ffmpeg-python", | |
| "pygmt", | |
| "pygmt-helper>=2025.12.1", | |
| "source-modelling>=2025.12.1", | |
| "workflow @ git+https://github.com/ucgmsim/workflow.git@pegasus", | |
| "qcore-utils>=2025.12.2", | |
| "geopandas", | |
| "pandas", | |
| "shapely", | |
| "scipy >= 1.15.0", | |
| "pooch", | |
| "typer", | |
| "tqdm", | |
| ] | |
| dependencies = [ | |
| "cartopy", | |
| "ffmpeg-python", | |
| "geopandas", | |
| "matplotlib", | |
| "numpy", | |
| "pandas", | |
| "pooch", | |
| "pygmt", | |
| "pygmt-helper>=2025.12.1", | |
| "qcore-utils>=2025.12.2", | |
| "scipy >= 1.15.0", | |
| "shapely", | |
| "source-modelling>=2025.12.1", | |
| "tqdm", | |
| "typer", | |
| "workflow @ git+https://github.com/ucgmsim/workflow.git@pegasus", | |
| ] |
| test = [ | ||
| "diffimg", | ||
| "pytest", | ||
| "pytest-cov", | ||
| "hypothesis[pandas, numpy]", | ||
| "hypothesis-networkx", | ||
| ] |
There was a problem hiding this comment.
The pytest-xdist dependency, which was present in the old requirements.txt, seems to be missing from the new test optional dependencies. Was its removal intentional? If not, it should be added back to ensure tests can be run in parallel. For consistency and maintainability, it's also a good idea to keep this list sorted alphabetically.
| test = [ | |
| "diffimg", | |
| "pytest", | |
| "pytest-cov", | |
| "hypothesis[pandas, numpy]", | |
| "hypothesis-networkx", | |
| ] | |
| test = [ | |
| "diffimg", | |
| "hypothesis[pandas, numpy]", | |
| "hypothesis-networkx", | |
| "pytest", | |
| "pytest-cov", | |
| "pytest-xdist", | |
| ] |
There was a problem hiding this comment.
Pull request overview
This PR migrates the realisation plotting module from using PGV (Peak Ground Velocity) targets to using rrup (rupture distance) boundary polygons for domain determination, aligning with a codebase-wide change made several months ago. Additionally, the PR consolidates dependency management by removing requirements.txt and moving all dependencies into pyproject.toml.
Changes:
- Updated
plot_realisation()to accept individual configuration objects instead of a realisation file path, and replaced PGV-based polygon generation with rrup-based polygon generation usinggenerate_domain.find_r_surfaces() - Removed
requirements.txtand migrated all dependencies topyproject.tomlwith proper separation of main, test, types, and dev dependencies - Simplified the GitHub Actions deptry workflow to use
uvinstead of manual pip installations
Reviewed changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| visualisation/realisation.py | Refactored plotting functions to use rrup boundaries instead of PGV targets; updated function signatures to accept configuration objects directly instead of file paths; renamed parameters from pgv_target to rrup_target |
| requirements.txt | Deleted in favor of pyproject.toml |
| pyproject.toml | Added dependencies section with all packages from requirements.txt; added optional-dependencies for test, types, and dev; added plot-realisation script entry point; minor formatting improvements |
| .github/workflows/deptry.yml | Simplified workflow to use uv for dependency installation and checking |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| The region of the plot. | ||
| pgv_target : float | ||
| rrup_target : float | ||
| The PGV target for the polygon (used as a label). |
There was a problem hiding this comment.
The docstring still references "The PGV target for the polygon" when describing the rrup_target parameter. This should be updated to say "The rrup target (distance in meters) for the polygon" to accurately reflect what this parameter represents.
| The PGV target for the polygon (used as a label). | |
| The rrup target (distance in meters) for the polygon (used as a label). |
A few months ago we migrated to using rrup boundary polygons for domain determination (again). So this module needed an update. Also included: deleting the requirements.txt, adding a lock file, and refactoring the dependencies into pyproject.toml.