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

Changes from 1 commit
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
25 changes: 8 additions & 17 deletions pygmt_helper/plotting.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,7 @@ 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")
Expand All @@ -300,31 +301,21 @@ def gen_region_fig(
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)
# 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 +370,14 @@ def gen_region_fig(
continuous=plot_kwargs["topo_cmap_continous"],
cmap=plot_kwargs["topo_cmap"],
reverse=plot_kwargs["topo_cmap_reverse"],
no_bg=True
)

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

# Plot inland water
Expand Down
Loading