Skip to content
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

pygmt.dataset.load_*: Add type hints for the 'region' parameter #3272

Merged
merged 5 commits into from
May 27, 2024

Conversation

seisman
Copy link
Member

@seisman seisman commented May 25, 2024

Description of proposed changes

In the previous version, region can be either a list or a string, but a string like region="0/10/0/20" is not Pythonic and a tuple like region=(0, 10, 0, 20) is also accepted.

This PR adds type hints to the region parameter of load_* functions. The region parameter must be a Sequence type (list, tuple, or array). A string type is still supported but no longer documented/recommended.

Preview: https://pygmt-dev--3272.org.readthedocs.build/en/3272/api/generated/pygmt.datasets.load_earth_age.html#pygmt.datasets.load_earth_age

@seisman seisman added skip-changelog Skip adding Pull Request to changelog needs review This PR has higher priority and needs review. typing Type hints and static type checking labels May 25, 2024
@seisman
Copy link
Member Author

seisman commented May 25, 2024

Will apply the same changes to other load_* functions if @GenericMappingTools/pygmt-maintainers agree with the changes.

Edit: Applied to all load_functions in 4ddf1f9


__doctest_skip__ = ["load_earth_age"]


@kwargs_to_strings(region="sequence")
Copy link
Member Author

Choose a reason for hiding this comment

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

This line can be removed because region is passed to _load_remote_dataset and in that function region can be a sequence.

@seisman seisman added this to the 0.13.0 milestone May 25, 2024
@seisman seisman added final review call This PR requires final review and approval from a second reviewer and removed needs review This PR has higher priority and needs review. labels May 27, 2024
Copy link
Member

@yvonnefroehlich yvonnefroehlich left a comment

Choose a reason for hiding this comment

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

Passing a list or a tuple is definitely more Pythonic.
But I am wondering how we want to handle arguments containing ISO codes and arguments extending a region via appending +r, e.g., lon_min/lon_ma/lat_min/lat_max+rincrement or ISO_code+rincrement. Here, we still need a string input.

@seisman
Copy link
Member Author

seisman commented May 27, 2024

But I am wondering how we want to handle arguments containing ISO codes and arguments extending a region via appending +r, e.g., lon_min/lon_ma/lat_min/lat_max+rincrement or ISO_code+rincrement. Here, we still need a string input.

That's a good point.

lon_min/lon_ma/lat_min/lat_max+rincrement

I don't think GMT's -R option has such syntax (see https://docs.generic-mapping-tools.org/dev/gmt.html#r-full). -Rxlleft/ylleft/xuright/yuright+r is close to this one, but it's mainly for specifying a region for oblique projections. So, I guess the syntax doesn't make sense here.

ISO_code+rincrement

This one should be supported currently. We probably should implement the ideas in #2646 and then people should use the proposed to dcw module to get the region before passing to the region parameter.

Anyway, I'll add the string type back. Added back in 9c2ac53.

@yvonnefroehlich
Copy link
Member

lon_min/lon_ma/lat_min/lat_max+rincrement

I don't think GMT's -R option has such syntax (see https://docs.generic-mapping-tools.org/dev/gmt.html#r-full). -Rxlleft/ylleft/xuright/yuright+r is close to this one, but it's mainly for specifying a region for oblique projections. So, I guess the syntax doesn't make sense here.

You are totally right; such a syntax is not available, and your guess is what I was actually thinking about. I must have gotten confused during writing - I apologize for the confusion!

@seisman seisman merged commit 14082d8 into main May 27, 2024
19 checks passed
@seisman seisman deleted the datasets/region branch May 27, 2024 23:50
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label May 27, 2024
weiji14 added a commit that referenced this pull request Sep 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog Skip adding Pull Request to changelog typing Type hints and static type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants