Skip to content

Figure.coast/pygmt.select/pygmt.grdlandmask: Use long names ("crude"/"low"/"intermediate"/"high"/"full") for the 'resolution' parameter - #3013

Merged
seisman merged 16 commits into
mainfrom
coast/resolution
Jun 3, 2025
Merged

Figure.coast/pygmt.select/pygmt.grdlandmask: Use long names ("crude"/"low"/"intermediate"/"high"/"full") for the 'resolution' parameter#3013
seisman merged 16 commits into
mainfrom
coast/resolution

Conversation

@seisman

@seismanseisman commented Jan 18, 2024

Copy link
Copy Markdown
Member

This PR updates the resolution parameter for Figure.coast/pygmt.select/pygmt.grdlandmask to use long names.

@seismanseisman changed the title POC: coast: Make the 'resolution' parameter more PythonicFigure.coast: Use long names ("crude"/"low"/"intermediate"/"high"/"full") for the 'resolution' parameterJul 21, 2024
@seismanseisman changed the title Figure.coast: Use long names ("crude"/"low"/"intermediate"/"high"/"full") for the 'resolution' parameterFigure.coast/pygmt.select/pygmt.grdlandmask: Use long names ("crude"/"low"/"intermediate"/"high"/"full") for the 'resolution' parameterJul 22, 2024
@seisman
seisman marked this pull request as ready for review October 30, 2024 01:23
@seismanseisman added needs review This PR has higher priority and needs review. run/benchmark Trigger the benchmark workflow in PRs enhancement Improving an existing feature labels Oct 30, 2024
@codspeed-hq

codspeed-hqBot commented Oct 30, 2024

Copy link
Copy Markdown

CodSpeed Performance Report

Merging #3013 will not alter performance

Comparing coast/resolution (50f6406) with main (438edce)

Summary

✅ 104 untouched benchmarks

Comment threadpygmt/src/select.py Outdated
Comment threadpygmt/src/grdlandmask.py
Comment threadpygmt/src/coast.py
@seisman

seisman commented Oct 30, 2024

Copy link
Copy Markdown
MemberAuthor

Ping @GenericMappingTools/pygmt-maintainers for thoughts on the proposed changes towards more Pythonic arguments.

@seismanseisman self-assigned this Oct 30, 2024
@seisman

Copy link
Copy Markdown
MemberAuthor

Actually, this PR is not needed if PR #3238 is implemented.

@seismanseisman removed the needs review This PR has higher priority and needs review. label Oct 30, 2024
@seisman
seisman marked this pull request as draft October 30, 2024 11:21
@seismanseisman added this to the 0.16.0 milestone May 24, 2025
@seismanseisman added the needs review This PR has higher priority and needs review. label May 24, 2025
@seisman
seisman marked this pull request as ready for review May 24, 2025 06:23
@seisman

Copy link
Copy Markdown
MemberAuthor

Actually, this PR is not needed if PR #3238 is implemented.

It's likely PR #3238 will take months to implement, so I feel it's better to have this PR merged before that PR>

Comment threadpygmt/src/_common.py
Comment on lines +256 to +258
def _parse_coastline_resolution(
resolution: Literal["auto", "full", "high", "intermediate", "low", "crude", None],
) -> Literal["a", "f", "h", "i", "l", "c", None]:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thoughts on making a StrEnum out of this?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

What are the benefits? This private function will likely be removed after #3239 is implemented.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

GMT uses short-form arguments, but we prefer long-form arguments in PyGMT. Another example is in #3012. It will be a lot of work if we define StrEnum for all these cases. #3239 proposed a more general solution for it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, StrEnum will be a lot more work, and we'll need to bring in something like pydantic (mentioned at #3239 (comment)) to fully enable runtime checks. Ok to keep this as is for now, and wait for #3239.

Comment threadpygmt/src/coast.py
Comment on lines +214 to +215
kwargs["D"] = kwargs.get("D", _parse_coastline_resolution(resolution))

@seismanseismanMay 25, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The simplest form of this line is:

kwargs["D"] = _parse_coastline_resolution(resolution)

but to support short-form parameters like D="f", we have to write the code like:

kwargs["D"] = kwargs.get("D", _parse_coastline_resolution(resolution))

The D="resolution" line is also removed from the use_alias decorator. Otherwise, we have to write codes like below:

kwargs["D"] = _parse_coastline_resolution(kwgars.get("D"))

It's fine, but then ruff will complain that the resolution parameter is unused.

Comment threadpygmt/src/coast.py
A="area_thresh",
B="frame",
C="lakes",
D="resolution",

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

After removing D="resolution", this alias won't be shown in the alias list. Need to wait for #3945.

@seisman
seisman merged commit 06dd1a5 into mainJun 3, 2025
@seisman
seisman deleted the coast/resolution branch June 3, 2025 04:59
@seismanseisman removed needs review This PR has higher priority and needs review. run/benchmark Trigger the benchmark workflow in PRs labels Jun 3, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancementImproving an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@seisman@weiji14