Skip to content

Fix figure generation on GMT 6.5+ #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lispandfound merged 10 commits into from
Aug 27, 2025
Merged

Fix figure generation on GMT 6.5+ #13

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5b22f89
fix(plotting): don't use transparency for water colour
lispandfound Aug 25, 2025
da31374
fix(plotting): ruff fixes
lispandfound Aug 25, 2025
90b7d26
Update pygmt_helper/plotting.py
lispandfound Aug 25, 2025
a9369dd
docs(plotting): document `no_bg=True`
lispandfound Aug 26, 2025
be05ba7
Merge branch 'topo_fix' of github.com:ucgmsim/pygmt_helper into topo_fix
lispandfound Aug 26, 2025
90a44bb
fix: remove shapely dependency
lispandfound Aug 26, 2025
0610015
fix(plotting): escape codes
lispandfound Aug 26, 2025
3bef088
fix(plotting): correctly set title and subtitle
lispandfound Aug 26, 2025
37306d4
fix(plotting): apply NaN-colour only for gen_region_fig
lispandfound Aug 26, 2025
dce1754
refactor(ratio): update ratio example to be kinder on your computer
lispandfound Aug 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 17 additions & 27 deletions pygmt_helper/plotting.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import pygmt
import xarray as xr
from scipy import interpolate
from shapely import geometry
from qcore import point_in_polygon

GMT_DATA = pooch.create(
Expand Down Expand Up @@ -290,41 +289,30 @@ def gen_region_fig(
# Merge with default
plot_kwargs = copy.deepcopy(DEFAULT_PLT_KWARGS) | (plot_kwargs or {})

if title:
plot_kwargs["frame_args"] = plot_kwargs.get("frame_args", []) + [
f"+t{title}".replace(" ", r"\040")
]

if subtitle:
plot_kwargs["frame_args"] = plot_kwargs.get("frame_args", []) + [
f"+s{subtitle}".replace(" ", r"\040")
]
config_options = {"COLOR_NAN": plot_kwargs["water_color"]} | (config_options or {})
pygmt.config(**config_options)
Copy link

@gemini-code-assist gemini-code-assist bot Aug 25, 2025

Choose a reason for hiding this comment

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

medium

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 fig

This change would make the function more robust and self-contained.

Copy link
Contributor Author

@lispandfound lispandfound Aug 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@claudio525 claudio525 Aug 26, 2025

Choose a reason for hiding this comment

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

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.


if fig is None:
fig = pygmt.Figure()

if config_options:
pygmt.config(**config_options)

water_color = plot_kwargs["water_color"]
plot_kwargs["frame_args"] = plot_kwargs.get("frame_args", []) + [f"+g{water_color}"]
fig.basemap(
region=region if region else "NZ",
projection=projection,
frame=plot_kwargs["frame_args"],
)

# Plot coastline and background water
bg_region = region if region else fig.region
water_bg = geopandas.GeoSeries(
geometry.LineString(
[
(bg_region[0], bg_region[2]),
(bg_region[1], bg_region[2]),
(bg_region[1], bg_region[3]),
[bg_region[0], bg_region[3]],
]
)
)
fig.plot(water_bg, fill=plot_kwargs["water_color"], straight_line=True)
title_args = []
if title:
title_args.append(f"+t{title}")
if subtitle:
title_args.append(f"+s{subtitle}")

if title:
Copy link

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

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

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.

Suggested change
if title:
if title_args:

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@lispandfound lispandfound Aug 26, 2025

Choose a reason for hiding this comment

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

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.

fig.basemap(frame=title_args)

# Plot coastline
fig.plot(
data=map_data.coastline_df,
pen=f"{plot_kwargs['coastline_pen_width']}p,{plot_kwargs['coastline_pen_color']}",
Expand Down Expand Up @@ -379,14 +367,16 @@ def gen_region_fig(
continuous=plot_kwargs["topo_cmap_continous"],
cmap=plot_kwargs["topo_cmap"],
reverse=plot_kwargs["topo_cmap_reverse"],
# Some CPTs define their own COLOR_NAN, but we wish to use the
# water colour
no_bg=True,
)

# Plot topography
fig.grdimage(
grid=topo_grid,
shading=topo_shading_grid,
cmap=True,
nan_transparent=True,
)

# Plot inland water
Expand Down
1 change: 0 additions & 1 deletion requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ pyarrow
pygmt
xarray
scipy
shapely
qcore @ git+https://github.com/ucgmsim/qcore.git
pytest
pytest-cov
Expand Down