Wrap the GMT API function GMT_Read_Data to read data into GMT data containers #3324

seisman · 2024-07-10T07:04:45Z

Description of proposed changes

See #3318 for the motivations.

This PR wraps the GMT API functions GMT_Read_Data for reading data file into GMT data containers. Currently, only GMT_DATASET and GMT_GRID are supported, other data containers will be supported later.

The first two commits 5a56ba8 and 7ccf641 are cherry-picked from PR #3318.

Preview at https://pygmt-dev--3324.org.readthedocs.build/en/3324/api/generated/pygmt.clib.Session.read_data.html

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 wrapping a new module, open a 'Wrap new GMT module' issue and submit reasonably-sized PRs.
If adding new functionality, add an example to docstrings or tutorials.
Use underscores (not hyphens) in names of Python files and directories.

Slash Commands

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

/format: automatically format and lint the code

…ntainers

… need cast is needed outside the function

…r dataarray

seisman · 2024-07-10T10:24:59Z

pygmt/tests/test_clib_read_data.py

+        data_ptr = lib.read_data(infile, kind="grid", mode="GMT_CONTAINER_ONLY")
+        grid = data_ptr.contents
+        header = grid.header.contents
+        assert header.n_rows == 14
+        assert header.n_columns == 8
+        assert header.wesn[:] == [-55.0, -47.0, -24.0, -10.0]
+        assert header.z_min == 190.0
+        assert header.z_max == 981.0
+        assert header.n_bands == 1  # Explicitely check n_bands
+        assert not grid.data  # The data is not read yet


Can't do xrgrid = data_ptr.contents.to_dataarray() here because data is not read yet, so grid.data is NULL. Maybe we should modify the GMT_GRID.to_dataarray() method to fill the DataArray with np.empty() if grid.data is not read yet?

Edit: I think it's not necessary after thinking twice.

pygmt/clib/session.py

pygmt/tests/test_clib_read_data.py

weiji14 · 2024-07-18T23:21:23Z

pygmt/clib/session.py

+        mode
+            How the data is to be read from the file. This option varies depending on
+            the given family. See the GMT API documentation for details.


Can you link to where in the GMT API documentation what options there are for mode?

mode is explained at https://docs.generic-mapping-tools.org/dev/devdocs/api.html#import-from-a-file-stream-or-handle.

pygmt/tests/test_clib_read_data.py

Co-authored-by: Wei Ji <[email protected]>

weiji14 · 2024-07-19T04:02:39Z

pygmt/clib/session.py

+        kind: Literal["dataset", "grid"],
+        family: str | None = None,
+        geometry: str | None = None,
+        mode: str = "GMT_READ_NORMAL",


Thinking on whether to make this an enum or use Literal instead of a str. Looking at https://docs.generic-mapping-tools.org/6.5/devdocs/api.html#import-from-a-file-stream-or-handle, it seems like it's possible to pass in mode as one of the variants of GMT_enum_container or GMT_enum_gridio? It also looks like mode can be None (or unset) if reading from text or table.

I believe the documentation is incomplete, and mode can accept specific values plus some modifiers, making it impossible to list all of them.

Yeah that what I thought too, there might be other options we's unaware of. That said, should we allow for mode=None (or whatever works when we don't want to set a mode)?

In the GMT C API, mode has an unsigned int type, so the mode argument must be an integer number and can't be None. The C enum GMT_READ_NORMAL has a value of 0 (https://github.com/GenericMappingTools/gmt/blob/3e9fcb1f54e4e93fc1e309e0be6e2288eb79f3d2/src/gmt_resources.h#L244), which is also the default read mode.

We can define mode=None, and add one line code like:

if mode == None: mode = "GMT_READ_NORMAL"

but I feel it's not as good as setting GMT_READ_NORMAL as the default.

Ah ok then, then the GMT_READ_NORMAL default here makes sense. Maybe good to mention it in the docstring.

weiji14

Thanks @seisman, just one additional suggestion on documenting the default mode="GMT_READ_NORMAL". Also need to fix the W505 doc-line-too-long lint error before merging.

weiji14 · 2024-07-19T06:04:59Z

pygmt/clib/session.py

+        kind: Literal["dataset", "grid"],
+        family: str | None = None,
+        geometry: str | None = None,
+        mode: str = "GMT_READ_NORMAL",


Ah ok then, then the GMT_READ_NORMAL default here makes sense. Maybe good to mention it in the docstring.

pygmt/clib/session.py

seisman added 2 commits July 10, 2024 15:01

Wrap the GMT API function GMT_Read_Data to read data into GMT data co…

5a56ba8

…ntainers

Add tests for reading datasets and grids

7ccf641

seisman added the feature Brand new feature label Jul 10, 2024

seisman added this to the 0.13.0 milestone Jul 10, 2024

seisman added 5 commits July 10, 2024 15:05

Add the new API function to docs

024728d

Refactor to make the function definition more Pythonic

afd8dc7

Let the function return the pointer to the actual data containers, so…

8dfb24a

… need cast is needed outside the function

No need to call _parse_constant

76a511e

Simplify the tests by converting GMT data containers into dataframe o…

591172b

…r dataarray

seisman force-pushed the gmt-read-data branch from 46d72e8 to 591172b Compare July 10, 2024 09:16

seisman added 2 commits July 10, 2024 17:58

Further simplify the tests

3d8ccb6

Simple checks if rioxarray is not installed

d1c6249

seisman commented Jul 10, 2024

View reviewed changes

seisman added 4 commits July 11, 2024 11:43

Let mode default to 'GMT_READ_NORMAL'

b4b034c

Rename the parameter from wesn to region

5c036fa

Merge branch 'main' into gmt-read-data

6144cfd

Improve docstrings

8f8494c

seisman marked this pull request as ready for review July 11, 2024 03:56

Add one more test to catch exceptions

6206cdb

seisman added the needs review This PR has higher priority and needs review. label Jul 11, 2024

seisman mentioned this pull request Jul 11, 2024

POC: Wrap GMT_Read_Data and read datasets/grids/images into GMT data container #3318

Closed

5 tasks

seisman commented Jul 11, 2024

View reviewed changes

pygmt/clib/session.py Outdated Show resolved Hide resolved

seisman added 3 commits July 11, 2024 17:31

Fix a comment

fad4cce

Merge branch 'main' into gmt-read-data

d65f7f9

Allow setting family and geometry manually

bc3494e

seisman requested a review from a team July 17, 2024 06:15

weiji14 reviewed Jul 18, 2024

View reviewed changes

seisman and others added 3 commits July 19, 2024 09:03

Fix.typos

9a6628d

Co-authored-by: Wei Ji <[email protected]>

Fix a typo

5bf40f7

Co-authored-by: Wei Ji <[email protected]>

Add link to the upstream documentation

85fad50

Explicitly set engine to rasterio

3a63040

Co-authored-by: Wei Ji <[email protected]>

weiji14 reviewed Jul 19, 2024

View reviewed changes

weiji14 approved these changes Jul 19, 2024

View reviewed changes

seisman added 3 commits July 19, 2024 14:15

Merge branch 'main' into gmt-read-data

b3cb4bc

Fix styling issues

8009581

Explain GMT_READ_NORMAL in docstrings

cc8ed2b

seisman removed the needs review This PR has higher priority and needs review. label Jul 19, 2024

seisman merged commit 917b3aa into main Jul 19, 2024

seisman deleted the gmt-read-data branch July 19, 2024 06:28

Wrap the GMT API function GMT_Read_Data to read data into GMT data containers #3324

Wrap the GMT API function GMT_Read_Data to read data into GMT data containers #3324

Uh oh!

Conversation

seisman commented Jul 10, 2024 • edited by weiji14 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seisman Jul 10, 2024 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weiji14 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

seisman commented Jul 10, 2024 •

edited by weiji14

Loading

seisman Jul 10, 2024 •

edited

Loading