Refactor info and grdinfo to use virtualfile_from_data #961

weiji14 · 2021-02-24T04:03:21Z

Description of proposed changes

Create a universal virtualfile_from_data function that can handle any raster or vector data input. This allows us to centralize the data validation logic in a single place, resulting in a cleaner API for PyGMT modules to handle different PyData types (e.g. numpy/pandas/xarray/etc) seamlessly. This is the beginning of resolving #949.

Preview API docs at https://pygmt-git-virtualfilefromdata-gmt.vercel.app/api/generated/pygmt.clib.Session.virtualfile_from_data.html

Before:

kind = data_kind(grid, None, None)
with Session() as lib:
    if kind == "file":
        file_context = dummy_context(grid)
    elif kind == "grid":
        file_context = lib.virtualfile_from_grid(grid)
    else:
        raise GMTInvalidInput("Unrecognized data type: {}".format(type(grid)))
    with file_context as infile:
        ...

After

with Session() as lib:
    file_context = lib.virtualfile_from_data(check_kind="raster", data=grid)

As a start, both info and grdinfo have been refactored to use this new convenience function. Other modules will be refactored in separate PRs later.

Addresses #949.

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

Create a universal `virtualfile_from_data` function that can handle any raster or vector data input. This allows us to centralize the data validation logic in a single place, resulting in a cleaner API for PyGMT modules to handle different PyData types (e.g. numpy/pandas/xarray/etc) seamlessly. As a start, both `info` and `grdinfo` have been refactored to use this new convenience function.

seisman · 2021-02-24T05:31:10Z

pygmt/clib/session.py

@@ -1359,6 +1360,62 @@ def virtualfile_from_grid(self, grid):
        with self.open_virtual_file(*args) as vfile:
            yield vfile

+    def virtualfile_from_data(self, data, x=None, y=None, z=None, check_kind=None):


A quick question: does it work for modules that requires more than three columns? For example, in Figure.plot(), we need to pass extra_arrays to GMT API.

Excellent point. Perhaps put the check_kind parameter in front, so that it becomes virtualfile_from_data(self, check_kind, data, x, y, z, *extra_arrays)?

I'm thinking if having two functions can make it easier:

virtualfile_from_table_data: data can be file or matrix, or x, y, z, extra_arrays can be vector.

virtualfile_from_grid_data: data can be file, or xr.DataArray

Not necessarily easier for the developer coding this up! I prefer one universal data I/O function, but if we must split it, then virtualfile_from_vector and virtualfile_from_raster sounds better.

I'm OK with one universal function, but you may want to look at some more complicated modules (e.g., grdtrack and plot). In these modules, it seems the data kind is needed outside the virtualfile_from_data function.

Yeah, grdtrack needs a good refactor, and so does plot. I think we can refactor this virtualfile_from_data function when we refactor those complicated modules, so need to make sure the implementation here is ok to be extended to those functions.

pygmt/clib/session.py

seisman · 2021-02-25T06:43:29Z

pygmt/clib/session.py

@@ -1359,6 +1360,62 @@ def virtualfile_from_grid(self, grid):
        with self.open_virtual_file(*args) as vfile:
            yield vfile

+    def virtualfile_from_data(self, data, x=None, y=None, z=None, check_kind=None):


I'm OK with one universal function, but you may want to look at some more complicated modules (e.g., grdtrack and plot). In these modules, it seems the data kind is needed outside the virtualfile_from_data function.

Pure dictionary based lookup to convert data to a virtualfile.

Fixes all the tests which expect first input into data_kind to be the 'data' argument.

Revert e558834 somewhat, because the dictionary actually creates four virtualfiles. Also pushed up 'file' and 'grid' kinds up the if-then block chain.

seisman · 2021-03-02T21:40:06Z

There are maybe two lines not hit by the tests (see https://codecov.io/gh/GenericMappingTools/pygmt/src/7c60cb304950aacb0ed0e1b3c76fac24a242b65d/pygmt/clib/session.py).

weiji14 · 2021-03-02T21:43:02Z

There are maybe two lines not hit by the tests (see https://codecov.io/gh/GenericMappingTools/pygmt/src/7c60cb304950aacb0ed0e1b3c76fac24a242b65d/pygmt/clib/session.py).

Yes I'm aware of them. They will be covered when I refactor other functions (e.g. plot, grdtrack) to use virtualfile_from_data, so just a temporary thing.

seisman

OK to merge.

weiji14 · 2021-03-02T21:45:00Z

Cool, just want to make sure the full tests pass and I'll merge after that.

The virtualfile_from_data function wrapped in #961 allows for a standardized way for inputting data to PyGMT modules. This change is for standardizing some of the docstrings, specifically for 'table-like' vector inputs which can be of several different PyData object types.

The virtualfile_from_data function wrapped in #961 allows for a standardized way for inputting data to PyGMT modules. This change is for standardizing some of the docstrings, specifically for 'table-like' vector inputs which can be of several different PyData object types. * Reformat pygmt.info docstring to use table-like filler * Reformat plot and plot3d docstring to use table-like filler * Reformat data_kind and virtualfile_from_data docstring to use table-like filler * Reformat rose docstring to use table-like filler * Reformat grdtrack docstring to use table-like filler * Reformat velo docstring to use table-like filler * Reformat wiggle docstring to use table-like filler * Reformat histogram docstring to use table-like filler * Add Python list to histogram's table param description * Use table-classes filler in docstring description * Use fmt_docstring decorator on virtualfile_from_data function Also had to remove curly brackets from doctest so that they are not treated as placeholders to be substituted. * Add a doctest for the table-classes filler text Co-authored-by: Meghan Jones <[email protected]> Co-authored-by: Dongdong Tian <[email protected]>

…gTools#961) Create a universal `virtualfile_from_data` function that can handle any raster or vector data input. This allows us to centralize the data validation logic in a single place, resulting in a cleaner API for PyGMT modules to handle different PyData types (e.g. numpy/pandas/xarray/etc) seamlessly. As a start, both `info` and `grdinfo` have been refactored to use this new convenience function. * Move check_kind to be the first parameter * Move check_kind out of data_kind function and into virtualfile_from_data Fixes all the tests which expect first input into data_kind to be the 'data' argument. * Add doctest example usage of virtualfile_from_data with xarray.Dataset * Add an entry to doc/api/index.rst for clib.Session.virtualfile_from_data

The virtualfile_from_data function wrapped in GenericMappingTools#961 allows for a standardized way for inputting data to PyGMT modules. This change is for standardizing some of the docstrings, specifically for 'table-like' vector inputs which can be of several different PyData object types. * Reformat pygmt.info docstring to use table-like filler * Reformat plot and plot3d docstring to use table-like filler * Reformat data_kind and virtualfile_from_data docstring to use table-like filler * Reformat rose docstring to use table-like filler * Reformat grdtrack docstring to use table-like filler * Reformat velo docstring to use table-like filler * Reformat wiggle docstring to use table-like filler * Reformat histogram docstring to use table-like filler * Add Python list to histogram's table param description * Use table-classes filler in docstring description * Use fmt_docstring decorator on virtualfile_from_data function Also had to remove curly brackets from doctest so that they are not treated as placeholders to be substituted. * Add a doctest for the table-classes filler text Co-authored-by: Meghan Jones <[email protected]> Co-authored-by: Dongdong Tian <[email protected]>

weiji14 added the enhancement Improving an existing feature label Feb 24, 2021

seisman reviewed Feb 24, 2021

View reviewed changes

seisman reviewed Feb 25, 2021

View reviewed changes

Move check_kind to be the first parameter

ebd2264

vercel bot temporarily deployed to Preview February 26, 2021 00:34 Inactive

Refactor virtualfile_from_data to remove if/elif/else block

e558834

Pure dictionary based lookup to convert data to a virtualfile.

vercel bot temporarily deployed to Preview February 26, 2021 01:07 Inactive

weiji14 added 2 commits February 26, 2021 14:16

Move check_kind out of data_kind function and into virtualfile_from_data

febc06c

Fixes all the tests which expect first input into data_kind to be the 'data' argument.

Go back to using an if/elif block so only one virtualfile is created

9bdd0be

Revert e558834 somewhat, because the dictionary actually creates four virtualfiles. Also pushed up 'file' and 'grid' kinds up the if-then block chain.

vercel bot temporarily deployed to Preview February 26, 2021 01:34 Inactive

Reduce diff on data_kind change

4b5e8df

vercel bot temporarily deployed to Preview February 26, 2021 01:41 Inactive

weiji14 mentioned this pull request Feb 26, 2021

Integration with geopandas to plot shapely geometries #608

Closed

weiji14 added 2 commits March 2, 2021 10:42

Merge branch 'master' into virtualfile_from_data

4761fad

Add doctest example usage of virtualfile_from_data with xarray.Dataset

23530f4

vercel bot temporarily deployed to Preview March 1, 2021 21:43 Inactive

weiji14 added this to the 0.3.1 milestone Mar 1, 2021

weiji14 requested a review from a team March 1, 2021 21:52

Add an entry to doc/api/index.rst for clib.Session.virtualfile_from_data

7c60cb3

vercel bot temporarily deployed to Preview March 2, 2021 01:18 Inactive

seisman approved these changes Mar 2, 2021

View reviewed changes

weiji14 marked this pull request as ready for review March 2, 2021 21:38

Merge branch 'master' into virtualfile_from_data

c6ed35c

vercel bot temporarily deployed to Preview March 2, 2021 21:38 Inactive

seisman approved these changes Mar 2, 2021

View reviewed changes

weiji14 merged commit 6d57b64 into master Mar 2, 2021

weiji14 deleted the virtualfile_from_data branch March 2, 2021 22:10

weiji14 mentioned this pull request Mar 3, 2021

Universal virtualfile_from_data function to replace virtualfile_from_grid/matrix/vectors #949

Closed

6 tasks

seisman added maintenance Boring but important stuff for the core devs and removed enhancement Improving an existing feature labels Mar 9, 2021

weiji14 mentioned this pull request Apr 6, 2021

Standardize docstrings for table-like inputs #1186

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor info and grdinfo to use virtualfile_from_data #961

Refactor info and grdinfo to use virtualfile_from_data #961

weiji14 commented Feb 24, 2021 •

edited

Loading

seisman Feb 24, 2021

weiji14 Feb 24, 2021

seisman Feb 24, 2021 •

edited

Loading

weiji14 Feb 24, 2021

seisman Feb 25, 2021

weiji14 Feb 26, 2021

seisman Feb 26, 2021

seisman Feb 25, 2021

seisman commented Mar 2, 2021

weiji14 commented Mar 2, 2021

seisman left a comment

weiji14 commented Mar 2, 2021

Refactor info and grdinfo to use virtualfile_from_data #961

Refactor info and grdinfo to use virtualfile_from_data #961

Conversation

weiji14 commented Feb 24, 2021 • edited Loading

Before:

After

seisman Feb 24, 2021

Choose a reason for hiding this comment

weiji14 Feb 24, 2021

Choose a reason for hiding this comment

seisman Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

weiji14 Feb 24, 2021

Choose a reason for hiding this comment

seisman Feb 25, 2021

Choose a reason for hiding this comment

weiji14 Feb 26, 2021

Choose a reason for hiding this comment

seisman Feb 26, 2021

Choose a reason for hiding this comment

seisman Feb 25, 2021

Choose a reason for hiding this comment

seisman commented Mar 2, 2021

weiji14 commented Mar 2, 2021

seisman left a comment

Choose a reason for hiding this comment

weiji14 commented Mar 2, 2021

weiji14 commented Feb 24, 2021 •

edited

Loading

seisman Feb 24, 2021 •

edited

Loading