Skip to content

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

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.

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

seisman
Copy link
Member

@seisman seisman commented Jan 18, 2024

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

@seisman seisman force-pushed the coast/resolution branch from d9af3e0 to 88be372 Compare July 21, 2024 09:53
@seisman seisman changed the title POC: coast: Make the 'resolution' parameter more Pythonic Figure.coast: Use long names ("crude"/"low"/"intermediate"/"high"/"full") for the 'resolution' parameter Jul 21, 2024
@seisman seisman changed the title Figure.coast: Use long names ("crude"/"low"/"intermediate"/"high"/"full") for the 'resolution' parameter Figure.coast/pygmt.select/pygmt.grdlandmask: Use long names ("crude"/"low"/"intermediate"/"high"/"full") for the 'resolution' parameter Jul 22, 2024
@seisman seisman marked this pull request as ready for review October 30, 2024 01:23
@seisman seisman 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
Copy link

codspeed-hq bot commented Oct 30, 2024

CodSpeed Performance Report

Merging #3013 will not alter performance

Comparing coast/resolution (39987aa) with main (e890738)

Summary

✅ 104 untouched benchmarks

@seisman
Copy link
Member Author

seisman commented Oct 30, 2024

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

@seisman seisman self-assigned this Oct 30, 2024
@seisman
Copy link
Member Author

seisman commented Oct 30, 2024

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

@seisman seisman 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
@seisman seisman added this to the 0.16.0 milestone May 24, 2025
@seisman seisman 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
Member Author

seisman commented May 24, 2025

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 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
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
Member Author

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
Member Author

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.

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

Copy link
Member Author

@seisman seisman May 25, 2025

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.

@@ -20,7 +23,6 @@
A="area_thresh",
B="frame",
C="lakes",
D="resolution",
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improving an existing feature needs review This PR has higher priority and needs review. run/benchmark Trigger the benchmark workflow in PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants