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 addresses a transparency issue in pygmt-helper when used with GMT versions 6.5 and newer. It resolves the problem by replacing the problematic nan_transparent option with a more robust approach that utilizes GMT's COLOR_NAN configuration and frame arguments to define the water background color, ensuring proper rendering of topography over water.
Highlights
- Fixes Transparency Issues: Resolves rendering problems with topography over water in
pygmt-helperwhen using GMT versions 6.5.0 and 6.6.0. - Switches Water Background Method: Replaces the buggy
nan_transparentoption withCOLOR_NANand+gframe arguments for setting water color. - Ensures GMT Compatibility: Improves compatibility and stability with newer GMT releases by avoiding a known buggy feature.
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 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
-
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.
Code Review
This PR fixes a transparency issue in recent GMT versions by replacing the buggy nan_transparent option with COLOR_NAN and a frame background color. The changes are logical and effectively address the problem. My review includes one suggestion to improve code robustness by scoping the GMT configuration changes to avoid side effects.
pygmt_helper/plotting.py
Outdated
| ] | ||
|
|
||
| config_options = {'COLOR_NAN': plot_kwargs['water_color']} | (config_options or {}) | ||
| pygmt.config(**config_options) |
There was a problem hiding this comment.
Modifying the global GMT configuration directly can lead to unexpected side effects in other parts of the code that might use pygmt. It is safer to use a context manager (with pygmt.config(...)) to temporarily apply these settings only for the duration of this function's execution. This will prevent the configuration from 'leaking' and affecting other plots.
This would require some restructuring. For example:
def gen_region_fig(...):
# ... (setup code before config)
config_options = {'COLOR_NAN': plot_kwargs['water_color']} | (config_options or {})
if fig is None:
fig = pygmt.Figure()
with pygmt.config(**config_options):
# ... (all plotting logic using fig and pygmt)
return figThis change would make the function more robust and self-contained.
There was a problem hiding this comment.
Yeah I tossed up using this in a with context before making the PR. In my opinion, this is not a good idea because many of the config options are set in gen_region_fig with the expectation that they persist outside the invocation of the function. For example, fonts. I also thought about just setting the COLOR_NAN locally and everything else globally. @claudio525 what do you think? Would the user be surprised if the NaN colour is the ocean? Maybe it will make them think something is wrong?
There was a problem hiding this comment.
I think this is fine as is for now, it matches with the current implementation. If this turns out to be an issue down the line, then I think we do a refactor, and change all config_options to be applied using a context manager, but don't think there is a good reason to do this now.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
claudio525
left a comment
There was a problem hiding this comment.
This looks great, nice one!
Can you also update the plot_grid function to not use nan_transparent (
pygmt_helper/pygmt_helper/plotting.py
Line 507 in fa8af50
Since the
gen_region_fig happens before, and COLOR_NAN will be set, this should just work (I hope). The only exception is when the nan is used for masking something other than water, but I think thats a bridge we can cross when we get to it. Should be easy to check with the ratio.py example.
pygmt_helper/plotting.py
Outdated
| ] | ||
|
|
||
| config_options = {'COLOR_NAN': plot_kwargs['water_color']} | (config_options or {}) | ||
| pygmt.config(**config_options) |
There was a problem hiding this comment.
I think this is fine as is for now, it matches with the current implementation. If this turns out to be an issue down the line, then I think we do a refactor, and change all config_options to be applied using a context manager, but don't think there is a good reason to do this now.
|
I don't necessarily know if disabling I suspect
I have also gone with the half-way approach I outlined earlier regarding |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes transparency issues in pygmt-helper for GMT versions 6.5+ by replacing the buggy nan_transparent option with a more robust approach using COLOR_NAN and frame arguments to set water color behind topography.
- Replaces
nan_transparent=TruewithCOLOR_NANconfiguration for handling water areas - Simplifies water background plotting by using frame background color instead of geometric shapes
- Updates example code to use improved grid spacing and output format
Reviewed Changes
Copilot reviewed 2 out of 5 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| pygmt_helper/plotting.py | Core fix implementing COLOR_NAN approach and simplifying water background rendering |
| pygmt_helper/examples/ratio.py | Example updates demonstrating improved grid spacing and output format changes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if subtitle: | ||
| title_args.append(f"+s{subtitle}") | ||
|
|
||
| if title: |
There was a problem hiding this comment.
The condition if title: is checked twice (lines 307 and 312), but the second check should be if title_args: to avoid calling basemap with empty frame args when only subtitle is provided.
| if title: | |
| if title_args: |
There was a problem hiding this comment.
I knew the AI would flag this. Nope it is intentional. See the GMT docs
+ttitle to place the string given in title centered above the plot frame [default is no title].
+ssubtitle (requires +ttitle) to place the string given in subtitle beneath the title [default is no subtitle].
Note that +s requires +t, ergo subtitle= requires title=. Thus checking if title_args is non-empty leaves open the subtle bug that a user might call subtitle= and not title=. Hence, we check if title is not None. In that case, title_args is also not None. Likewise, if title is None then we should ignore the values in title_args even if subtitle is set.

This PR fixes transparency issues for the latest versions of GMT (6.5.0 and the new 6.6.0). Currently,
pygmt-helperuses thenan_transparent(gmt option-Q) option to plot topography over water. Unfortunately, this is a buggy option in recent versions of GMT. This PR switches to using theCOLOR_NANand frame arguments to set water colour behind the topography. Because it doesn't use transparency, new versions of GMT work just fine with it.