Wrap gmtselect by weiji14 · Pull Request #1429 · GenericMappingTools/pygmt

weiji14 · 2021-08-10T11:23:44Z

Description of proposed changes

Wrapping the gmtselect function which "Selects data table subsets based on multiple spatial criteria". For a GMT example, see https://docs.generic-mapping-tools.org/6.2/gallery/ex24.html.

Preview docs at https://pygmt-git-wrap-gmtselect-gmt.vercel.app/api/generated/pygmt.select.html

Crossref GMT.jl docs at https://www.generic-mapping-tools.org/GMT.jl/v0.31/#GMT.gmtselect.

Parameters/Aliases to wrap:

To keep this PR small, I would prefer implementing only 4 out of 7 of the possible gmtselect filters, specifically -A (once #1426 is done), -R rectangular region subset, -D/-N coastline subset, and -Z z value data subset. These, plus the reverse (-I) and grid subset (-G) option. The others can be done in a follow-up PR.

Other implementation points to consider (may need to be done in other PRs):

GMT's select function supports multiple file inputs, should we support this in PyGMT? See e.g. text at 7a08ebd. Related to this is if we should support multiple table-like inputs (e.g. two geopandas.GeoDataFrame objects in a list).
I've used the long alias z_subregion for -Z (following pygmt.grdcut, xref Wrap grdcut #492). Should we use zsubregion (no underscore) instead, following policy set in https://github.com/GenericMappingTools/pygmt/blame/v0.4.1/doc/contributing.md#L446-L450)?
Using the region -R and -J common aliases here in select doesn't look good since it's not a plotting function. Probably don't need the Required if this is the first plot command part. Edit: Will be resolved by Add region docstring to COMMON_OPTIONS for non-plotting modules #1493/Update region and projection standard docstrings #1510
![image](https://user-images.githubusercontent.com/23487320/128861719-c17dffdb-026e-4d7b-a30d-ff01bf4766d9.png

Addresses first part of #1427.

Reminders

Run make format and make check to make sure the code follows the style guide.
Add tests for new features or tests that would have caught the bug that you're fixing.
Add new public functions/methods/classes to doc/api/index.rst.
Write detailed docstrings for all functions/methods.
If adding new functionality, add an example to docstrings or tutorials.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

/format: automatically format and lint the code
/test-gmt-dev: run full tests on the latest GMT development version

Initial commit for wrapping the gmtselect function for #1427 which selects data table subsets based on multiple spatial criteria. Original GMT `gmtselect` documentation is at https://docs.generic-mapping-tools.org/6.2/gmtselect.html. Aliased non-common optional parameters reverse (I) and z_subregion (Z).

willschlitzer · 2021-10-05T18:35:43Z

@weiji14 Is the intent just to have the output of this as the file or a DataFrame, or should their be an option for a numpy array as well?

weiji14 · 2021-10-05T18:41:26Z

@weiji14 Is the intent just to have the output of this as the file or a DataFrame, or should their be an option for a numpy array as well?

Yeah, maybe just a file or pandas.DataFrame for now, until someone makes a reusable function to handle #1318.

willschlitzer · 2021-10-05T19:21:10Z

pygmt/src/select.py

+        Pass in either a file name to an ASCII data table, a 2D
+        {table-classes}.


Suggested change

Pass in either a file name to an ASCII data table, a 2D

{table-classes}.

Pass in either a file name to an ASCII data table or a 2D

{table-classes}.

I"m not familiar with using gmtselect, but can't it accept 3D tables (as indicated by z_subregion?

This docstring gets rendered like so:

So 2D numpy.array is correct. I don't think it's common to speak of a 3D table (though I did find https://stackoverflow.com/questions/24290495/constructing-3d-pandas-dataframe), but I know what you mean by having a z-column.

Maybe something like "2D or 3D {table-classes}"? I understand that 3D tables aren't the norm, but it doesn't make sense to have a parameter specifically for a 3D table but then say that gmtselect only accepts a 2D table.

This is what I get when I search for 3D data table:

If we are talking about a 3D numpy array, the shape will be something like (1, 2, 3), but in this case, our input cannot be like that. It can only be a 2D shape like (2, 3). The extra z-column isn't considered an extra dimension, more like an extra attribute.

Okay! That makes sense to me; I was thinking a 3D table meant one with data for the third dimension.

willschlitzer · 2021-10-05T19:22:33Z

pygmt/src/select.py

+    {A}
+    reverse : str
+        [**cflrsz**].
+        Reverses the sense of the test for each of the criteria specified:


Suggested change

Reverses the sense of the test for each of the criteria specified:

Reverses the criteria of the test for each of the criteria specified:

The use of "sense" doesn't make much sense to me. I know it's from the GMT docs, but would there be a better way to phrase this?

Yeah, it doesn't sound nice, but I think the following statements ("select records NOT inside/within ...") should (hopefully) make it clear to users what is meant by reverse. So maybe just keep it as with the GMT docs?

If it's easier to keep it in line with the GMT docs, I'm fine with it. But I might open a PR over at GMT; I don't think the current version makes much sense.

willschlitzer · 2021-10-25T08:20:54Z

@weiji14 Are you intending to get this pull request complete for v0.5.0?

weiji14 · 2021-10-25T22:26:47Z

@weiji14 Are you intending to get this pull request complete for v0.5.0?

Yes, I've added this PR to the priority list at #1576 (comment).

pygmt/src/select.py

Specifically, resolution (D), gridmask (G) and mask (N). These aliases are currently undocumented/disabled, but will be implemented/enabled in the future.

pygmt/src/select.py

* Wrap the gmtselect function which selects data table subsets based on multiple spatial criteria. Aliased non-common optional parameters reverse (I) and z_subregion (Z), area_thresh (A), resolution (D), gridmask (G) and mask (N). *Add tests for select *Add imports for select Co-authored-by: Will Schlitzer <schlitzer90@gmail.com>

weiji14 added the feature Brand new feature label Aug 10, 2021

weiji14 added this to the 0.5.0 milestone Aug 10, 2021

weiji14 self-assigned this Aug 10, 2021

weiji14 force-pushed the wrap/gmtselect branch from 253163b to 53ef119 Compare August 10, 2021 11:29

vercel bot temporarily deployed to Preview August 10, 2021 11:29 Inactive

weiji14 added 2 commits August 12, 2021 22:06

Merge branch 'main' into wrap/gmtselect

2a983ab

Alias area_thresh (A) for select

35ea6b8

vercel bot temporarily deployed to Preview August 12, 2021 10:13 Inactive

Merge branch 'main' into wrap/gmtselect

8ffb2d0

vercel bot temporarily deployed to Preview August 24, 2021 07:54 Inactive

maxrjones mentioned this pull request Sep 11, 2021

Standardize parameter name for table data inputs #1479

Closed

weiji14 added 2 commits September 27, 2021 21:23

Merge branch 'main' into wrap/gmtselect

84bba27

Rename 'table' parameter to 'data'

f8051ad

vercel bot temporarily deployed to Preview September 27, 2021 08:28 Inactive

weiji14 added 2 commits October 3, 2021 22:19

Merge branch 'main' into wrap/gmtselect

375c599

Parse incols and outcols arguments using sequence_comma

b6a8584

vercel bot temporarily deployed to Preview October 3, 2021 09:24 Inactive

weiji14 added 2 commits October 3, 2021 23:50

Merge branch 'main' into wrap/gmtselect

88240bd

Use area_thresh instead of -A in docstring for reverse param

d2902ee

vercel bot temporarily deployed to Preview October 3, 2021 10:52 Inactive

weiji14 requested a review from a team October 3, 2021 11:04

willschlitzer reviewed Oct 5, 2021

View reviewed changes

Merge branch 'main' into wrap/gmtselect

f11e95e

weiji14 marked this pull request as ready for review October 25, 2021 22:25

vercel bot temporarily deployed to Preview October 25, 2021 22:25 Inactive

weiji14 mentioned this pull request Oct 25, 2021

Release PyGMT v0.5.0 #1576

Closed

35 tasks

willschlitzer reviewed Oct 26, 2021

View reviewed changes

pygmt/src/select.py Outdated Show resolved Hide resolved

Use long aliases instead of short param in description of reverse param

96a41bf

Specifically, resolution (D), gridmask (G) and mask (N). These aliases are currently undocumented/disabled, but will be implemented/enabled in the future.

vercel bot temporarily deployed to Preview October 27, 2021 10:18 Inactive

willschlitzer reviewed Oct 27, 2021

View reviewed changes

pygmt/src/select.py Outdated Show resolved Hide resolved

weiji14 added 2 commits October 28, 2021 11:03

Merge branch 'main' into wrap/gmtselect

7eef59f

Alias resolution (D), gridmask (G) and mask (N) for gmtselect

4d003de

vercel bot temporarily deployed to Preview October 27, 2021 22:13 Inactive

willschlitzer approved these changes Oct 28, 2021

View reviewed changes

willschlitzer added the final review call This PR requires final review and approval from a second reviewer label Oct 28, 2021

Merge branch 'main' into wrap/gmtselect

e204dda

vercel bot temporarily deployed to Preview October 28, 2021 22:25 Inactive

Merge branch 'main' into wrap/gmtselect

1911a34

vercel bot temporarily deployed to Preview October 29, 2021 03:59 Inactive

willschlitzer merged commit bc140cb into main Oct 29, 2021

willschlitzer deleted the wrap/gmtselect branch October 29, 2021 04:34

seisman removed the final review call This PR requires final review and approval from a second reviewer label Oct 29, 2021

seisman mentioned this pull request Mar 9, 2022

Wrapper for gmtselect #1427

Closed

		Pass in either a file name to an ASCII data table, a 2D
		{table-classes}.

	Reverses the sense of the test for each of the criteria specified:
	Reverses the criteria of the test for each of the criteria specified:

Conversation

weiji14 commented Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willschlitzer commented Oct 5, 2021

Uh oh!

weiji14 commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willschlitzer Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

weiji14 Oct 8, 2021

Choose a reason for hiding this comment

Uh oh!

willschlitzer Oct 23, 2021

Choose a reason for hiding this comment

Uh oh!

weiji14 Oct 24, 2021

Choose a reason for hiding this comment

Uh oh!

willschlitzer Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

willschlitzer Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

weiji14 Oct 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willschlitzer Oct 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willschlitzer commented Oct 25, 2021

Uh oh!

weiji14 commented Oct 25, 2021

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

weiji14 commented Aug 10, 2021 •

edited

Loading

weiji14 commented Oct 5, 2021 •

edited

Loading

weiji14 Oct 8, 2021 •

edited

Loading

willschlitzer Oct 23, 2021 •

edited

Loading