Refactor the data_kind and validate_data_input functions #3335

seisman · 2024-07-17T09:33:28Z

Description of proposed changes

This PR is a subset of PR #2744. PR #2744 takes more time than I initially expected (Still not satisfied with the new Session.virtualfile_in API function), so better to refactor the data_kind/validate_data_input function in a separate PR (this PR).

As mentioned in PR #2744:

The data_kind function does three things: (1) determines the kind of the input data, and (2) checks if the data/x/y/z combinations are valid; and (3) checks if the matrix-type data has 3 columns.
The data_kind function is called inside Session.virtualfile_in, but sometimes we need to know the input data kind when wrapping GMT modules, for example, in Figure.plot and Figure.plot3d. It means the data_kind function is called twice, which is not necessary.

This PR moves the validate_data_input function outside of the data_kind, and calls it in Session.virtualfile_in explicitly. In this way, each function only focuses on one single data. which makes the functions easier to maintain.

I also don't like the current function design of the data_kind/validate_data_input functions, but I prefer to refactor the function details in a separate PR, to make this PR smaller for reviewing.

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

…data_input doctests

pygmt/helpers/utils.py

seisman · 2024-07-18T02:54:07Z

pygmt/helpers/utils.py

@@ -193,15 +188,6 @@ def data_kind(data=None, x=None, y=None, z=None, required_z=False, required_data
        kind = "matrix"
    else:
        kind = "vectors"
-    _validate_data_input(


Removed from here and moved to Session.virtualfile_in.

seisman · 2024-07-18T02:54:50Z

pygmt/helpers/utils.py

@@ -111,7 +115,9 @@ def _validate_data_input(
                raise GMTInvalidInput("data must provide x, y, and z columns.")


-def data_kind(data=None, x=None, y=None, z=None, required_z=False, required_data=True):
+def data_kind(


Now this function only checks data, and x/y/z is no longer used.

seisman · 2024-07-18T02:56:08Z

pygmt/tests/test_helpers.py

@@ -32,25 +25,6 @@ def test_load_static_earth_relief():
    assert isinstance(data, xr.DataArray)


-@pytest.mark.parametrize(


This test is removed because:

data_kind no longer checks if the input is valid

All these tests are already covered by the validate_data_input doctests.

weiji14 · 2024-07-19T04:08:52Z

pygmt/helpers/utils.py

@@ -124,64 +130,53 @@ def data_kind(data=None, x=None, y=None, z=None, required_z=False, required_data
    * 1-D arrays x and y (and z, optionally)


Delete this line about x/y/z?

Suggested change

* 1-D arrays x and y (and z, optionally)

Updated the docstring at b575591. Currently, any unrecognized data type is taken as a matrix type. It's not ideal, so I plan to refactor the data_kind function later, and will also polish the docstring when refactoring.

weiji14 · 2024-07-19T04:12:48Z

pygmt/helpers/utils.py

    'arg'
    >>> data_kind(data=xr.DataArray(np.random.rand(4, 3)))
    'grid'
    >>> data_kind(data=xr.DataArray(np.random.rand(3, 4, 5)))
    'image'
    """
-    # determine the data kind
+    kind: Literal["arg", "file", "geojson", "grid", "image", "matrix", "vectors"]


What is this line doing here?

To suppress the mypy error:

pygmt/helpers/utils.py:191: error: Incompatible return value type (got "str", expected "Literal['arg', 'file', 'geojson', 'grid', 'image', 'matrix', 'vectors']") [return-value]

Ah interesting. Probably should turn this into an enum, but leave that for another day.

weiji14 · 2024-07-19T04:17:42Z

pygmt/helpers/utils.py

    Check what kind of data is provided to a module.



Since x/y/z is no longer used, at L124-129, change the docstring to read:

Possible types (provided to `data`): * a file name * a pathlib.PurePath object * an xarray.DataArray object * a 2-D matrix

weiji14 · 2024-07-19T04:18:03Z

pygmt/helpers/utils.py

@@ -124,64 +130,53 @@ def data_kind(data=None, x=None, y=None, z=None, required_z=False, required_data
    * 1-D arrays x and y (and z, optionally)
    * an optional argument (None, bool, int or float) provided as 'data'


Suggested change

* an optional argument (None, bool, int or float) provided as 'data'

* an optional argument (None, bool, int or float)

weiji14 · 2024-07-20T04:41:32Z

Do you know why the test_velo_numpy_array_numeric_only test is failing on Python 3.10/NumPy 1.24?

pygmt/pygmt/tests/test_velo.py

Lines 30 to 42 in d3101f3

    
           @pytest.mark.mpl_image_compare 
        
           def test_velo_numpy_array_numeric_only(dataframe): 
        
               """ 
        
               Plot velocity arrow and confidence ellipse from a numpy.ndarray. 
        
               """ 
        
               fig = Figure() 
        
               fig.velo( 
        
                   data=dataframe.iloc[:, :-1].to_numpy(), 
        
                   spec="e0.2/0.39/18", 
        
                   vector="0.3c+p1p+e+gred", 
        
                   frame="1g1", 
        
               ) 
        
               return fig

seisman · 2024-07-20T05:15:04Z

Do you know why the test_velo_numpy_array_numeric_only test is failing on Python 3.10/NumPy 1.24?

pygmt/pygmt/tests/test_velo.py

Lines 30 to 42 in d3101f3

@pytest.mark.mpl_image_compare

def test_velo_numpy_array_numeric_only(dataframe):

"""

Plot velocity arrow and confidence ellipse from a numpy.ndarray.

"""

fig = Figure()

fig.velo(

data=dataframe.iloc[:, :-1].to_numpy(),

spec="e0.2/0.39/18",

vector="0.3c+p1p+e+gred",

frame="1g1",

)

return fig

It passes in the latest rerun.

Make _validate_data_input public and move it to Session.virtualfile_in

e79a7d0

seisman force-pushed the refactor/data-kind branch from 48e1378 to e79a7d0 Compare July 17, 2024 09:36

seisman added 2 commits July 17, 2024 17:43

Remove test_data_kind_fails because it's already covered by validate_…

e2a8ceb

…data_input doctests

Remove the unused x/y/z/required_z parameters from data_kind

008a56f

seisman force-pushed the refactor/data-kind branch from 1316a94 to 008a56f Compare July 17, 2024 17:29

data_kind: Change the parameter name from required_data to required

46fb307

seisman force-pushed the refactor/data-kind branch from 9211a90 to 46fb307 Compare July 17, 2024 18:17

Update docstrings

d3854da

seisman commented Jul 18, 2024

View reviewed changes

Fix a typing hints issue

a9670aa

seisman marked this pull request as ready for review July 18, 2024 02:57

seisman added maintenance Boring but important stuff for the core devs skip-changelog Skip adding Pull Request to changelog needs review This PR has higher priority and needs review. run/benchmark Trigger the benchmark workflow in PRs labels Jul 18, 2024

seisman added this to the 0.13.0 milestone Jul 18, 2024

seisman added 2 commits July 19, 2024 09:50

Make validate_data_input private

7ff0d9e

Merge branch 'main' into refactor/data-kind

70962d0

weiji14 reviewed Jul 19, 2024

View reviewed changes

seisman added 3 commits July 19, 2024 13:06

Improve the docstrings of the data_kind function

b575591

Merge branch 'main' into refactor/data-kind

c522e36

Merge branch 'main' into refactor/data-kind

0bde17d

weiji14 approved these changes Jul 20, 2024

View reviewed changes

weiji14 removed the run/benchmark Trigger the benchmark workflow in PRs label Jul 20, 2024

seisman merged commit 9ec92be into main Jul 20, 2024
20 checks passed

seisman deleted the refactor/data-kind branch July 20, 2024 05:40

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the data_kind and validate_data_input functions #3335

Refactor the data_kind and validate_data_input functions #3335

seisman commented Jul 17, 2024 •

edited

Loading

seisman Jul 18, 2024

seisman Jul 18, 2024

seisman Jul 18, 2024

weiji14 Jul 19, 2024

seisman Jul 19, 2024

weiji14 Jul 19, 2024

seisman Jul 19, 2024

weiji14 Jul 20, 2024

weiji14 Jul 19, 2024

weiji14 Jul 19, 2024

weiji14 commented Jul 20, 2024

seisman commented Jul 20, 2024

		@@ -32,25 +25,6 @@ def test_load_static_earth_relief():
		assert isinstance(data, xr.DataArray)


		@pytest.mark.parametrize(

		@@ -124,64 +130,53 @@ def data_kind(data=None, x=None, y=None, z=None, required_z=False, required_data
		* 1-D arrays x and y (and z, optionally)

		@@ -124,64 +130,53 @@ def data_kind(data=None, x=None, y=None, z=None, required_z=False, required_data
		* 1-D arrays x and y (and z, optionally)
		* an optional argument (None, bool, int or float) provided as 'data'

Refactor the data_kind and validate_data_input functions #3335

Refactor the data_kind and validate_data_input functions #3335

Conversation

seisman commented Jul 17, 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

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 commented Jul 20, 2024

seisman commented Jul 20, 2024

seisman commented Jul 17, 2024 •

edited

Loading