Use a dynamically determined grid scale for a region based on size#106
Use a dynamically determined grid scale for a region based on size#106lispandfound merged 5 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @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 enhances the plotting capabilities by implementing a dynamic grid scaling mechanism for PyGMT plots. By adapting the grid resolution to the specific region being plotted, it aims to optimize performance and reduce plot generation times, particularly for large-scale SRF plots, moving away from a fixed, potentially inefficient, grid spacing.
Highlights
-
Dynamic Grid Scaling: A new utility function
grid_scale_for_regionhas been introduced to dynamically compute an optimal grid spacing for PyGMT plots based on the region's extent. -
Performance Improvement: The
plot_riseandplot_srffunctions now utilize this dynamic grid scale instead of a hardcoded value, which is expected to significantly reduce the time required to generate SRF plots, especially for larger geographical regions. -
Adaptive Resolution Formula: The grid scale is calculated using the formula
$\mathrm{max}(5, 2.5x)$ , where$x$ represents the largest extent (in kilometers) of the region in either latitude or longitude, ensuring a minimum resolution of 5 meters.
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| 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 issue 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 is currently in preview and 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 to provide feedback.
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
-
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. ↩
There was a problem hiding this comment.
Pull Request Overview
Adds a utility to compute a dynamic grid scale based on region size and updates SRF and rise plotting functions to use it instead of a hardcoded spacing.
- Introduce
grid_scale_for_regioninvisualisation/utils.py - Replace
"5e/5e"grid spacing withgrid_scalein SRF plots - Replace
"5e/5e"grid spacing withgrid_scalein rise plots
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| visualisation/utils.py | Added grid_scale_for_region to compute spacing based on region extents. |
| visualisation/sources/plot_srf.py | Use dynamic grid_scale instead of hardcoded 5e/5e; missing import needs adding. |
| visualisation/sources/plot_rise.py | Use dynamic grid_scale via plotting.grid_scale_for_region; namespace/import needs correcting. |
Comments suppressed due to low confidence (4)
visualisation/utils.py:297
- The return annotation is
floatbut the function always returns anint(and the docstring saysReturns int). Update the signature to-> intfor consistency.
def grid_scale_for_region(region: tuple[float, float, float, float]) -> float:
visualisation/utils.py:297
- [nitpick] Consider adding unit tests for
grid_scale_for_regionto verify output for known region sizes and edge cases (e.g., very small or large extents).
def grid_scale_for_region(region: tuple[float, float, float, float]) -> float:
visualisation/sources/plot_srf.py:122
- You need to import
grid_scale_for_region(e.g.from visualisation.utils import grid_scale_for_region) at the top of this file, otherwise this reference will be undefined.
grid_scale = grid_scale_for_region(region)
visualisation/sources/plot_rise.py:56
grid_scale_for_regionis not defined on theplottingmodule. Either import it fromvisualisation.utilsor call it directly if it's in scope.
grid_scale = plotting.grid_scale_for_region(region)
There was a problem hiding this comment.
Code Review
This pull request introduces a utility function to dynamically calculate grid scales for plots. The review identified a logical flaw in the grid_scale_for_region function, and incorrect calls to the function in plot_rise.py and plot_srf.py. A suggestion to align the function's type hint with its implementation was also provided.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
This small change adds a utility function$\mathrm{max}(5, 2.5x)$ , where $x$ is the largest extent (in km) of the region in latitude or longitude. Did this so I can make SRF plots without taking 20 minutes.
grid_scale_for_regionthat computes an appropriate grid scale for the region of the pygmt map which we use for the grid spacing inplotting.create_grid. The equation is