Skip to content

Figure.coast: Improve parameters lakes/river_lakes for setting fill of lakes/river-lakes #4376

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

Draft
seisman wants to merge 1 commit into
base: main
Choose a base branch
from
+69 −16
Draft

Figure.coast: Improve parameters lakes/river_lakes for setting fill of lakes/river-lakes #4376

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
85 changes: 69 additions & 16 deletions pygmt/src/coast.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,64 @@
from pygmt.alias import Alias, AliasSystem
from pygmt.clib import Session
from pygmt.exceptions import GMTInvalidInput
from pygmt.helpers import args_in_kwargs, build_arg_list, fmt_docstring, use_alias
from pygmt.helpers import (
args_in_kwargs,
build_arg_list,
fmt_docstring,
is_nonstr_iter,
use_alias,
)
from pygmt.params import Box

__doctest_skip__ = ["coast"]


def _alias_option_C(lakes=None, river_lakes=None): # noqa: N802
"""
Helper function to create the alias list for the -C option.

Example
-------
>>> def parse(**kwargs):
... return AliasSystem(C=_alias_option_C(**kwargs)).get("C")
>>> parse()
>>> parse(lakes="blue")
'blue'
>>> parse(river_lakes="cyan")
'cyan+r'
>>> parse(lakes="blue", river_lakes="cyan")
['blue+l', 'cyan+r']

>>> # Check for backward compatibility
>>> parse(lakes="blue+l")
'blue+l'
>>> parse(lakes="cyan+r")
'cyan+r'
>>> parse(lakes=["blue+l", "cyan+r"])
['blue+l', 'cyan+r']

>>> # Check for mixed usage error
>>> parse(lakes=["blue+l", "cyan+r"], river_lakes="cyan")
Traceback (most recent call last):
...
pygmt.exceptions.GMTInvalidInput: Parameter 'lakes' is given with a list; ...
"""
# Check for backward compatibility.
if is_nonstr_iter(lakes): # Old syntax: lakes is a list of strings.
if river_lakes is not None:
msg = "Parameter 'lakes' is given with a list; 'river_lakes' must be None."
raise GMTInvalidInput(msg)
return Alias(lakes, name="lakes") # Return as is.

# If only 'lakes' is specified, no suffix is needed.
return [
Alias(lakes, name="lakes", suffix="+l" if river_lakes is not None else ""),
Alias(river_lakes, name="river_lakes", suffix="+r"),
]


Comment on lines +58 to +66
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

_alias_option_C returns a list of Alias objects where both entries have no prefix (only suffix). When users pass short-form C=... via kwargs, AliasSystem.merge will build a warning/error message for a “sequence of aliases” that includes an empty “with optional parameters …” clause (see pygmt/alias.py around the _modifiers join). To avoid degraded/confusing messages for C, consider returning a single Alias (with its value computed as a string or list like ["blue+l", "cyan+r"]) instead of a list of Alias objects, or otherwise ensuring the merge message has meaningful content for this option.

Suggested change
return Alias(lakes, name="lakes") # Return as is.
# If only 'lakes' is specified, no suffix is needed.
return [
Alias(lakes, name="lakes", suffix="+l" if river_lakes is not None else ""),
Alias(river_lakes, name="river_lakes", suffix="+r"),
]
# Pass through the legacy list value unchanged as the argument to -C.
return Alias(lakes, name="C")
# New syntax: lakes and/or river_lakes are given as strings.
# We construct the final value for -C directly so that AliasSystem does not
# need to merge a sequence of aliases without prefixes.
values = []
if lakes is not None:
lake_val = str(lakes)
# If both lakes and river_lakes are specified and no modifier is present,
# append +l for lakes (for backward compatibility with the previous logic).
if (
river_lakes is not None
and "+l" not in lake_val
and "+r" not in lake_val
):
lake_val = f"{lake_val}+l"
values.append(lake_val)
if river_lakes is not None:
river_val = str(river_lakes)
# Append +r only if the user did not already include modifiers.
if "+r" not in river_val and "+l" not in river_val:
river_val = f"{river_val}+r"
values.append(river_val)
if not values:
# No value for -C.
return Alias(None, name="C")
combined = values[0] if len(values) == 1 else values
return Alias(combined, name="C")

Copilot uses AI. Check for mistakes.
@fmt_docstring
@use_alias(A="area_thresh", C="lakes", E="dcw")
@use_alias(A="area_thresh", E="dcw")
def coast( # noqa: PLR0913
self,
resolution: Literal[
Expand All @@ -26,6 +76,8 @@ def coast( # noqa: PLR0913
rivers: int | str | Sequence[int | str] | None = None,
borders: int | str | Sequence[int | str] | None = None,
shorelines: bool | str | Sequence[int | str] = False,
lakes: str | None = None,
river_lakes: str | None = None,
map_scale: str | None = None,
box: Box | bool = False,
projection: str | None = None,
Expand Down Expand Up @@ -59,6 +111,7 @@ def coast( # noqa: PLR0913

$aliases
- B = frame
- C = lakes, river_lakes
- D = resolution
- F = box
- G = land
Expand All @@ -74,18 +127,7 @@ def coast( # noqa: PLR0913

Parameters
----------
$projection
$region
*Required if this is the first plot command.*
$area_thresh
$frame
lakes : str or list
*fill*\ [**+l**\|\ **+r**].
Set the shade, color, or pattern for lakes and river-lakes. The
default is the fill chosen for "wet" areas set by the ``water``
parameter. Optionally, specify separate fills by appending
**+l** for lakes or **+r** for river-lakes, and passing multiple
strings in a list.
resolution
Select the resolution of the coastline dataset to use. The available resolutions
from highest to lowest are: ``"full"``, ``"high"``, ``"intermediate"``,
Expand All @@ -96,6 +138,11 @@ def coast( # noqa: PLR0913
Select filling of "dry" areas.
water
Select filling of "wet" areas.
lakes
river_lakes
Comment on lines +141 to +142
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

The docstring parameter entries for lakes/river_lakes don’t follow the numpydoc pattern used elsewhere in the file (missing : type on the name line and the shared description is indented under river_lakes only). This can break/garble rendered docs. Suggest documenting as either lakes, river_lakes : str with a single indented description, or provide separate lakes : str / river_lakes : str blocks (and mention the backward-compatible list-of-strings form if it remains supported).

Suggested change
lakes
river_lakes
lakes, river_lakes : str

Copilot uses AI. Check for mistakes.
Select filling of lakes and river-lakes. If not specified, will use the fill for
"wet" areas set by the ``water`` parameter. If ``lakes`` is specified but
``river_lakes`` isn't, ``river_lakes`` will use the same fill as ``lakes``.
Comment on lines +144 to +145
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

The docstring says “If lakes is specified but river_lakes isn’t, river_lakes will use the same fill as lakes.” This is true for the new API when lakes is a plain fill (emits -C<fill>), but it’s not true for the documented backward-compatible modifier forms like lakes="blue+l" (which intentionally affects lakes only). Consider clarifying that the inheritance behavior applies when lakes is given without an explicit +l/+r modifier.

Suggested change
"wet" areas set by the ``water`` parameter. If ``lakes`` is specified but
``river_lakes`` isn't, ``river_lakes`` will use the same fill as ``lakes``.
"wet" areas set by the ``water`` parameter. If ``lakes`` is specified as a plain
fill (without an explicit ``+l``/``+r`` modifier) but ``river_lakes`` isn't,
``river_lakes`` will use the same fill as ``lakes``.

Copilot uses AI. Check for mistakes.
rivers
Draw rivers. Specify the type of rivers to draw, and optionally append a pen
attribute, in the format *river*\ /*pen* [Default pen is
Expand Down Expand Up @@ -203,10 +250,14 @@ def coast( # noqa: PLR0913
to any of the continent codes (e.g. ``"=EU"`` for Europe). Append
**+p**\ *pen* to draw polygon outlines [Default is no outline] and
**+g**\ *fill* to fill them [Default is no fill].
$projection
$region
*Required if this is the first plot command.*
$frame
$verbose
$panel
$perspective
$transparency
$verbose

Example
-------
Expand Down Expand Up @@ -239,15 +290,17 @@ def coast( # noqa: PLR0913
and kwargs.get("I", rivers) is None
and kwargs.get("N", borders) is None
and kwargs.get("W", shorelines) is False
and not args_in_kwargs(args=["C", "E", "Q"], kwargs=kwargs)
and kwargs.get("C", lakes or river_lakes) is None
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

The “required args” validation uses kwargs.get("C", lakes or river_lakes) is None. Using or means a falsy but explicitly provided lakes value (e.g., "") would be treated as not provided. Using an explicit None check (e.g., prefer lakes if lakes is not None else river_lakes) avoids truthiness pitfalls and matches how the other checks treat values.

Suggested change
and kwargs.get("C", lakes or river_lakes) is None
and kwargs.get("C", lakes if lakes is not None else river_lakes) is None

Copilot uses AI. Check for mistakes.
and not args_in_kwargs(args=["E", "Q"], kwargs=kwargs)
):
msg = (
"At least one of the following parameters must be specified: "
"land, water, rivers, borders, shorelines, lakes, dcw, or Q."
"land, water, rivers, borders, shorelines, lakes, river_lakes, dcw, or Q."
)
raise GMTInvalidInput(msg)

aliasdict = AliasSystem(
C=_alias_option_C(lakes=lakes, river_lakes=river_lakes),
D=Alias(
Comment on lines 302 to 304
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

New behavior for lakes/river_lakes (including suffix selection and the backward-compatible lakes=["...+l", "...+r"] form and the mixed-usage error) isn’t covered by tests. pygmt/tests/test_coast.py currently tests required-args and dcw/resolution conflicts but has no assertions around -C argument building. Please add tests that exercise: (1) lakes="blue" emits -Cblue, (2) river_lakes="cyan" emits -Ccyan+r, (3) both emit two -C... entries with +l/+r, and (4) mixed list + river_lakes raises GMTInvalidInput.

Copilot uses AI. Check for mistakes.
resolution,
name="resolution",
Expand Down
Loading