clib: Add virtualfile_to_dataset method for converting virtualfile to a dataset #3083

seisman · 2024-03-01T12:50:34Z

Add the new Session.virtualfile_to_dataset() method to simplify the logic of dealing with table outputs in different wrappers. Draft PR #3092 shows how the new Session.virtualfile_to_dataset() method can simplify other wrappers.

seisman · 2024-03-07T06:06:12Z

pygmt/clib/session.py

@@ -1738,6 +1738,119 @@ def read_virtualfile(
 dtype = {"dataset": _GMT_DATASET, "grid": _GMT_GRID}[kind]
 return ctp.cast(pointer, ctp.POINTER(dtype))

+ def return_table(


The method name return_table was initially proposed in #1318 (comment), but is it a good name? At line 1845, we use kind="dataset", so maybe rename it to return_dataset or output_dataset? In the future, I think we will add more methods that return a grid/image/cpt/cube, so consistent method names are preferred.

I think return_dataset is better than return_table. Renamed in ce029b2.

How about virtualfile_to_dataset, or vfile_to_dataset (shorter)? A bit more explicit to say that a conversion is happening from a virtualfile to a tabular dataset.

virtualfile_to_dataset/vfile_to_dataset sounds a good name. I like vfile_to_dataset, but do we want to also rename other functions for consistency?

You mean rename the virtualfile_from_* methods? Let's maybe not do that (lazy to deprecate more names). We can go with virtualfile_to_dataset for consistency.

I meant to rename the virtualfile_in/virtualfile_out to vfile_in/vfile_out.

virtualfile_from_* functions are no longer used in our PyGMT wrappers after Universal virtualfile_from_data function to replace virtualfile_from_grid/matrix/vectors #949, so it's OK to keep them unchanged.

virtualfile_out was added in clib: Add the virtualfile_out method for creating output virtualfile #3057, so we can still rename it without deprecations.

virtualfile_in was renamed from virtualfile_from_data recently (PR clib: Rename the virtualfile_from_data method to virtualfile_in #3068), so we can rename it to vfile_in without more deprecations.

Let's keep with the long name (virtualfile_to_dataset). Renaming virtualfile_out -> vfile_out and virtualfile_in -> vfile_in will be more work and confusing (we'll need to manually update the changelog to track that virtualfile_from_data became virtualfile_in which became vfile_in).

Done in 711142c.

weiji14 · 2024-03-10T19:28:55Z

pygmt/clib/session.py

@@ -1738,6 +1738,123 @@ def read_virtualfile(
 dtype = {"dataset": _GMT_DATASET, "grid": _GMT_GRID}[kind]
 return ctp.cast(pointer, ctp.POINTER(dtype))

+ def return_dataset(
+ self,
+ output_type: Literal["pandas", "numpy", "file"],


Should we set a default output type here? It looks like we're using pandas as the default in #3092.

Suggested change

output_type: Literal["pandas", "numpy", "file"],

output_type: Literal["pandas", "numpy", "file"] = "pandas",

It makes no differences because we always call the function with the output_type parameter, e.g.,:

return lib.return_dataset( output_type=output_type, vfile=vouttbl, )

Yes, it doesn't make any difference in the PyGMT modules, but this is a good central location to document that output_type="pandas" is the default output (though in #1318, it seemed like most of us were in favour of output_type="input" or auto as the default).

output_type="input" or auto may not make sense for PyGMT, especially in cases like:

the input data is a file, then auto means outputting to a file by default, then outfile is required.

the input data is vectors (e.g., x/y/z) and each vector can be a list/ndarray/pd.Series. Then what's the expected format if auto/input is used?

Yeah, not saying that output_type="auto" would be easy to implement 🙂 I think the default output_type="pandas" is fine for now since it is an in-memory format that can be converted to virtualfiles relatively easily. We can discuss more about what the ideal output type would be in #1318 (if there is still any debate that needs to be had).

pygmt/clib/session.py

weiji14 · 2024-03-10T19:37:42Z

pygmt/clib/session.py

@@ -1738,6 +1738,119 @@ def read_virtualfile(
 dtype = {"dataset": _GMT_DATASET, "grid": _GMT_GRID}[kind]
 return ctp.cast(pointer, ctp.POINTER(dtype))

+ def return_table(


How about virtualfile_to_dataset, or vfile_to_dataset (shorter)? A bit more explicit to say that a conversion is happening from a virtualfile to a tabular dataset.

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

weiji14 · 2024-03-11T03:33:36Z

doc/api/index.rst

@@ -294,6 +294,7 @@ conversion of Python variables to GMT virtual files:
 clib.Session.virtualfile_from_grid
 clib.Session.virtualfile_in
 clib.Session.virtualfile_out
+ clib.Session.virtualfile_to_dataset


At L287 above, could you change the sentence to read "These methods are context managers that automate the conversion of Python variables to and from GMT virtual files"? Since we can convert GMT virtualfiles to Python objects now.

Done in aee0499.

seisman · 2024-03-11T04:13:40Z

In commit cd5b31d, I've changed the parameter name vfile to vfname to make it consistent with the virtualfile_out and read_virtualfile methods.

weiji14

Just one more small suggestion. Ok to leave out setting output_type="pandas" as the default setting (https://github.com/GenericMappingTools/pygmt/pull/3083/files#r1518966573) for now.

doc/api/index.rst

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

seisman · 2024-03-11T04:32:27Z

Thanks for all the feedbacks. Now marking the PR as "final review call" and will merge it after 24 hours.

seisman mentioned this pull request Mar 1, 2024

Wrap GMT's standard data type GMT_DATASET for table inputs #2729

Merged

2 tasks

clib: Add the return_table method for output table data

6827c82

seisman force-pushed the clib/return_table branch from a0d7ed0 to 6827c82 Compare March 7, 2024 03:01

Add the function to doc

bd166fe

seisman mentioned this pull request Mar 7, 2024

Make all functions/methods have consistent behavior for table output #3092

Closed

4 tasks

seisman added 3 commits March 7, 2024 13:22

Add doctest

d376a74

Improve docstrings

193bd05

fix

c9e482a

seisman commented Mar 7, 2024

View reviewed changes

seisman marked this pull request as ready for review March 7, 2024 06:18

seisman added enhancement Improving an existing feature needs review This PR has higher priority and needs review. labels Mar 7, 2024

seisman added this to the 0.12.0 milestone Mar 7, 2024

seisman added 2 commits March 8, 2024 07:49

Rename return_table to return_dataset

ce029b2

Update doc index page

9640c26

seisman changed the title ~~clib: Add the return_table method for table outputs~~ clib: Add the return_datasert method for table outputs Mar 8, 2024

seisman changed the title ~~clib: Add the return_datasert method for table outputs~~ clib: Add the return_dataset method for table outputs Mar 8, 2024

seisman requested a review from a team March 8, 2024 14:13

Merge branch 'main' into clib/return_table

9f9f08d

weiji14 reviewed Mar 10, 2024

View reviewed changes

seisman and others added 2 commits March 11, 2024 07:58

Update pygmt/clib/session.py

796f1cc

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

Rename return_dataset to virtualfile_to_dataset

711142c

seisman changed the title ~~clib: Add the return_dataset method for table outputs~~ clib: Add the virtualfile_to_dataset method for converting a virtualfile to a dataset Mar 11, 2024

weiji14 reviewed Mar 11, 2024

View reviewed changes

weiji14 changed the title ~~clib: Add the virtualfile_to_dataset method for converting a virtualfile to a dataset~~ clib: Add virtualfile_to_dataset method for converting virtualfile to a dataset Mar 11, 2024

seisman added 2 commits March 11, 2024 12:01

Improve the description about public methods

aee0499

Change parameter name vfile to vfname

cd5b31d

weiji14 approved these changes Mar 11, 2024

View reviewed changes

doc/api/index.rst Outdated Show resolved Hide resolved

Update doc/api/index.rst

bc7d844

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

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 Mar 11, 2024

michaelgrund approved these changes Mar 11, 2024

View reviewed changes

seisman merged commit 0b46aad into main Mar 11, 2024
19 of 21 checks passed

seisman deleted the clib/return_table branch March 11, 2024 11:17

seisman removed the final review call This PR requires final review and approval from a second reviewer label Mar 11, 2024

weiji14 mentioned this pull request Mar 16, 2024

Get rid of temporary files from pygmt functions and plotting methods #2730

Closed

31 tasks

This was referenced Mar 16, 2024

Wrap GMT's standard data type GMT_GRID for grids #2398

Merged

clib: Change the parameter order and set output_type to pandas in virtualfile_to_dataset #3124

Merged

seisman mentioned this pull request Apr 8, 2024

x2sys_cross: Refactor to get rid of temporary files and have consistent table-like output behavior #3160

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clib: Add virtualfile_to_dataset method for converting virtualfile to a dataset #3083

clib: Add virtualfile_to_dataset method for converting virtualfile to a dataset #3083

seisman commented Mar 1, 2024 •

edited by weiji14

Loading

seisman Mar 7, 2024

seisman Mar 7, 2024

weiji14 Mar 10, 2024 •

edited

Loading

seisman Mar 11, 2024

weiji14 Mar 11, 2024

seisman Mar 11, 2024

weiji14 Mar 11, 2024 •

edited

Loading

seisman Mar 11, 2024

weiji14 Mar 10, 2024

seisman Mar 10, 2024

weiji14 Mar 11, 2024

seisman Mar 11, 2024

weiji14 Mar 11, 2024 •

edited

Loading

weiji14 Mar 10, 2024 •

edited

Loading

weiji14 Mar 11, 2024 •

edited

Loading

seisman Mar 11, 2024

seisman commented Mar 11, 2024

weiji14 left a comment

seisman commented Mar 11, 2024

	output_type: Literal["pandas", "numpy", "file"],
	output_type: Literal["pandas", "numpy", "file"] = "pandas",

clib: Add virtualfile_to_dataset method for converting virtualfile to a dataset #3083

clib: Add virtualfile_to_dataset method for converting virtualfile to a dataset #3083

Conversation

seisman commented Mar 1, 2024 • edited by weiji14 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiji14 Mar 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiji14 Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiji14 Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

weiji14 Mar 10, 2024 • edited Loading

Choose a reason for hiding this comment

weiji14 Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seisman commented Mar 11, 2024

weiji14 left a comment

Choose a reason for hiding this comment

seisman commented Mar 11, 2024

seisman commented Mar 1, 2024 •

edited by weiji14

Loading

weiji14 Mar 10, 2024 •

edited

Loading

weiji14 Mar 11, 2024 •

edited

Loading

weiji14 Mar 11, 2024 •

edited

Loading

weiji14 Mar 10, 2024 •

edited

Loading

weiji14 Mar 11, 2024 •

edited

Loading