Repository: GenericMappingTools/pygmt

Issue:
Raise a GMTInvalidInput exception for only x,y input to blockmedian and blockmean
**Description of the desired feature**

Currently, no exception is raised in the `blockmedian` and `blockmean` functions if the `x` and `y` parameters are used but not the `z` parameter. This should be disallowed because these functions require three columns. For example, GMT will raise an error if a textfile with only two columns is used.

The simplest method to fix this problem would be to add the equivalent of these lines of code (using `table` rather than `data`) to the start of the `_blockm` function:
https://github.com/GenericMappingTools/pygmt/blob/cd822ca2c01f98a1496c63cc0bff8cc6661cb3bb/pygmt/src/surface.py#L81-L83

Another option could be to extend the `virtualfile_from_data` function to (optionally) check the number of columns. This would limit redundancy relative to using the same code block for all functions operating on x,y,z data (e.g., blockm*, surface, nearneighbor). If the test were in virtualfile_from_data, we could also reduce the tests required because the exception would not need to be tested in each module that requires three columns.

<!-- Please be as detailed as you can in your description. If possible, include an example of how you would like to use this feature (even better if it's a code example). -->


**Are you willing to help implement and maintain this feature?** Glad to help someone else with this issue

<!-- Every feature we add is code that we will have to maintain and keep updated. This takes a lot of effort. If you are willing to be involved in the project and help maintain your feature, it will make it easier for us to accept it. -->


Discussion:
yohaimagen: Where should I add tests for not providing the `z` vector?
yohaimagen: I added the test.

If we decide to go the route of requierd_z(see discussion at #1447) we should add `requierd_z = True` in all the functions that do require `z` column. Should we do it over here or in a separate PR for each function?
yohaimagen: /format
yohaimagen: I am making a list of functions that should pass `required_z=True` please add functions that I've missed. There are quite a lot of new functions that thas been wrapped recently and will be released in 0.5  please let me know which one of them needs to be set `required_z=True`.

- [ ] `plot3d`
- [ ]  `countour`
- [ ]  `blockmean`
- [ ] `blockmedian`
- [ ] `surface`

yohaimagen: Should we remove the tests of the relevant functions that test the passing of `z` column?  https://github.com/GenericMappingTools/pygmt/blob/cd822ca2c01f98a1496c63cc0bff8cc6661cb3bb/pygmt/tests/test_contour.py#L42-L56

if so we shuld extend the test to be similar to the above.
meghanrjones: > Should we remove the tests of the relevant functions that test the passing of `z` column?
> 
> https://github.com/GenericMappingTools/pygmt/blob/cd822ca2c01f98a1496c63cc0bff8cc6661cb3bb/pygmt/tests/test_contour.py#L42-L56
> 
> if so we shuld extend the test to be similar to the above.

Sorry for the slow response. Yes, my opinion is that invalid input tests for `data`, `x`, `y`, and `z` could be included in the `virtualfile_from_data` tests and removed from `surface`, `contour`, and `plot3d`.

Also, no need to take any action on this but just for explanation the contour code is different than the rest because it still needs refactoring for https://github.com/GenericMappingTools/pygmt/issues/949.

As for the other modules, here is a list:

- wiggle
- sphdistance (but hold off on this, since there's a PR open and currently no `x`, `y`, `z` option.)
yohaimagen: From my end its seems that this branch is ready to be merged into main, let me know if there is anything else to do over here.
yohaimagen: Actually, when writing the test for the case of a 2d array data with less than three columns when z is required, I noticed that we do not check that 2d array data have at least 2 columns in the general case, should we raise an error if a 2d matrix of a shape `nx1` is passed? and if so should I add it here or open another PR?
seisman: > Actually, when writing the test for the case of a 2d array data with less than three columns when z is required, I noticed that we do not check that 2d array data have at least 2 columns in the general case, should we raise an error if a 2d matrix of a shape `nx1` is passed? and if so should I add it here or open another PR?

It's more complicated, because for some modules (e.g., `histogrom`), only one column is required.
seisman: > > Actually, when writing the test for the case of a 2d array data with less than three columns when z is required, I noticed that we do not check that 2d array data have at least 2 columns in the general case, should we raise an error if a 2d matrix of a shape `nx1` is passed? and if so should I add it here or open another PR?
> 
> It's more complicated, because for some modules (e.g., `histogram`), only one column is required.

I think this PR is good for merge and we can deal with the 2-column data check in another issue/PR.

Patch:
diff --git a/pygmt/clib/session.py b/pygmt/clib/session.py
index a775a119..39977287 100644
--- a/pygmt/clib/session.py
+++ b/pygmt/clib/session.py
@@ -1362,7 +1362,14 @@ class Session:
 
     @fmt_docstring
     def virtualfile_from_data(
-        self, check_kind=None, data=None, x=None, y=None, z=None, extra_arrays=None
+        self,
+        check_kind=None,
+        data=None,
+        x=None,
+        y=None,
+        z=None,
+        extra_arrays=None,
+        required_z=False,
     ):
         """
         Store any data inside a virtual file.
@@ -1415,7 +1422,7 @@ class Session:
         ...
         <vector memory>: N = 3 <7/9> <4/6> <1/3>
         """
-        kind = data_kind(data, x, y, z)
+        kind = data_kind(data, x, y, z, required_z=required_z)
 
         if check_kind == "raster" and kind not in ("file", "grid"):
             raise GMTInvalidInput(f"Unrecognized data type for grid: {type(data)}")
diff --git a/pygmt/helpers/utils.py b/pygmt/helpers/utils.py
index fe4910c2..19b6e2ec 100644
--- a/pygmt/helpers/utils.py
+++ b/pygmt/helpers/utils.py
@@ -15,7 +15,7 @@ import xarray as xr
 from pygmt.exceptions import GMTInvalidInput
 
 
-def data_kind(data, x=None, y=None, z=None):
+def data_kind(data, x=None, y=None, z=None, required_z=False):
     """
     Check what kind of data is provided to a module.
 
@@ -69,7 +69,9 @@ def data_kind(data, x=None, y=None, z=None):
     if data is not None and (x is not None or y is not None or z is not None):
         raise GMTInvalidInput("Too much data. Use either data or x and y.")
     if data is None and (x is None or y is None):
-        raise GMTInvalidInput("Must provided both x and y.")
+        raise GMTInvalidInput("Must provide both x and y.")
+    if data is None and required_z and z is None:
+        raise GMTInvalidInput("Must provide x, y, and z.")
 
     if isinstance(data, (str, pathlib.PurePath)):
         kind = "file"
@@ -78,6 +80,8 @@ def data_kind(data, x=None, y=None, z=None):
     elif hasattr(data, "__geo_interface__"):
         kind = "geojson"
     elif data is not None:
+        if required_z and data.shape[1] < 3:
+            raise GMTInvalidInput("data must provide x, y, and z columns.")
         kind = "matrix"
     else:
         kind = "vectors"
diff --git a/pygmt/src/blockm.py b/pygmt/src/blockm.py
index a0484cfa..906ee5c0 100644
--- a/pygmt/src/blockm.py
+++ b/pygmt/src/blockm.py
@@ -43,7 +43,7 @@ def _blockm(block_method, table, outfile, x, y, z, **kwargs):
         with Session() as lib:
             # Choose how data will be passed into the module
             table_context = lib.virtualfile_from_data(
-                check_kind="vector", data=table, x=x, y=y, z=z
+                check_kind="vector", data=table, x=x, y=y, z=z, required_z=True
             )
             # Run blockm* on data table
             with table_context as infile:
diff --git a/pygmt/src/contour.py b/pygmt/src/contour.py
index 871ad994..ac5efe98 100644
--- a/pygmt/src/contour.py
+++ b/pygmt/src/contour.py
@@ -3,7 +3,6 @@ contour - Plot contour table data.
 """
 
 from pygmt.clib import Session
-from pygmt.exceptions import GMTInvalidInput
 from pygmt.helpers import (
     build_arg_string,
     data_kind,
@@ -127,9 +126,7 @@ def contour(self, x=None, y=None, z=None, data=None, **kwargs):
     """
     kwargs = self._preprocess(**kwargs)  # pylint: disable=protected-access
 
-    kind = data_kind(data, x, y, z)
-    if kind == "vectors" and z is None:
-        raise GMTInvalidInput("Must provided both x, y, and z.")
+    kind = data_kind(data, x, y, z, required_z=True)
 
     with Session() as lib:
         # Choose how data will be passed in to the module
diff --git a/pygmt/src/plot3d.py b/pygmt/src/plot3d.py
index eb8e91ea..89aadd62 100644
--- a/pygmt/src/plot3d.py
+++ b/pygmt/src/plot3d.py
@@ -235,7 +235,13 @@ def plot3d(
     with Session() as lib:
         # Choose how data will be passed in to the module
         file_context = lib.virtualfile_from_data(
-            check_kind="vector", data=data, x=x, y=y, z=z, extra_arrays=extra_arrays
+            check_kind="vector",
+            data=data,
+            x=x,
+            y=y,
+            z=z,
+            extra_arrays=extra_arrays,
+            required_z=True,
         )
 
         with file_context as fname:
diff --git a/pygmt/src/surface.py b/pygmt/src/surface.py
index 9079b8c0..24953b3f 100644
--- a/pygmt/src/surface.py
+++ b/pygmt/src/surface.py
@@ -94,9 +94,7 @@ def surface(x=None, y=None, z=None, data=None, **kwargs):
         - None if ``outgrid`` is set (grid output will be stored in file set by
           ``outgrid``)
     """
-    kind = data_kind(data, x, y, z)
-    if kind == "vectors" and z is None:
-        raise GMTInvalidInput("Must provide z with x and y.")
+    kind = data_kind(data, x, y, z, required_z=True)
 
     with GMTTempFile(suffix=".nc") as tmpfile:
         with Session() as lib:
diff --git a/pygmt/src/wiggle.py b/pygmt/src/wiggle.py
index b7434385..9c97858f 100644
--- a/pygmt/src/wiggle.py
+++ b/pygmt/src/wiggle.py
@@ -107,7 +107,7 @@ def wiggle(self, x=None, y=None, z=None, data=None, **kwargs):
     with Session() as lib:
         # Choose how data will be passed in to the module
         file_context = lib.virtualfile_from_data(
-            check_kind="vector", data=data, x=x, y=y, z=z
+            check_kind="vector", data=data, x=x, y=y, z=z, required_z=True
         )
 
         with file_context as fname:


Test patch:
diff --git a/pygmt/tests/test_clib.py b/pygmt/tests/test_clib.py
index f31a28fc..a7cc3bff 100644
--- a/pygmt/tests/test_clib.py
+++ b/pygmt/tests/test_clib.py
@@ -1,9 +1,11 @@
 # pylint: disable=protected-access
+# pylint: disable=redefined-outer-name
 """
 Test the wrappers for the C API.
 """
 import os
 from contextlib import contextmanager
+from itertools import product
 
 import numpy as np
 import numpy.testing as npt
@@ -23,11 +25,20 @@ from pygmt.exceptions import (
 from pygmt.helpers import GMTTempFile
 
 TEST_DATA_DIR = os.path.join(os.path.dirname(__file__), "data")
+POINTS_DATA = os.path.join(TEST_DATA_DIR, "points.txt")
 
 with clib.Session() as _lib:
     gmt_version = Version(_lib.info["version"])
 
 
+@pytest.fixture(scope="module")
+def data():
+    """
+    Load the point data from the test file.
+    """
+    return np.loadtxt(POINTS_DATA)
+
+
 @contextmanager
 def mock(session, func, returns=None, mock_func=None):
     """
@@ -410,6 +421,58 @@ def test_virtual_file_bad_direction():
                 print("This should have failed")
 
 
+def test_virtualfile_from_data_required_z_matrix():
+    """
+    Test that function fails when third z column in a matrix is needed but not
+    provided.
+    """
+    data = np.ones((5, 2))
+    with clib.Session() as lib:
+        with pytest.raises(GMTInvalidInput):
+            with lib.virtualfile_from_data(data=data, required_z=True):
+                pass
+
+
+def test_virtualfile_from_data_fail_non_valid_data(data):
+    """
+    Should raise an exception if too few or too much data is given.
+    """
+    # Test all combinations where at least one data variable
+    # is not given in the x, y case:
+    for variable in product([None, data[:, 0]], repeat=2):
+        # Filter one valid configuration:
+        if not any(item is None for item in variable):
+            continue
+        with clib.Session() as lib:
+            with pytest.raises(GMTInvalidInput):
+                lib.virtualfile_from_data(
+                    x=variable[0],
+                    y=variable[1],
+                )
+
+    # Test all combinations where at least one data variable
+    # is not given in the x, y, z case:
+    for variable in product([None, data[:, 0]], repeat=3):
+        # Filter one valid configuration:
+        if not any(item is None for item in variable):
+            continue
+        with clib.Session() as lib:
+            with pytest.raises(GMTInvalidInput):
+                lib.virtualfile_from_data(
+                    x=variable[0], y=variable[1], z=variable[2], required_z=True
+                )
+
+    # Should also fail if given too much data
+    with clib.Session() as lib:
+        with pytest.raises(GMTInvalidInput):
+            lib.virtualfile_from_data(
+                x=data[:, 0],
+                y=data[:, 1],
+                z=data[:, 2],
+                data=data,
+            )
+
+
 def test_virtualfile_from_vectors():
     """
     Test the automation for transforming vectors to virtual file dataset.
diff --git a/pygmt/tests/test_contour.py b/pygmt/tests/test_contour.py
index 3e285de9..8c083370 100644
--- a/pygmt/tests/test_contour.py
+++ b/pygmt/tests/test_contour.py
@@ -3,12 +3,10 @@
 Tests contour.
 """
 import os
-from itertools import product
 
 import numpy as np
 import pytest
 from pygmt import Figure
-from pygmt.exceptions import GMTInvalidInput
 
 TEST_DATA_DIR = os.path.join(os.path.dirname(__file__), "data")
 POINTS_DATA = os.path.join(TEST_DATA_DIR, "points.txt")
@@ -30,46 +28,6 @@ def region():
     return [10, 70, -5, 10]
 
 
-def test_contour_fail_no_data(data):
-    """
-    Should raise an exception if no data is given.
-    """
-    # Contour should raise an exception if no or not sufficient data
-    # is given
-    fig = Figure()
-    # Test all combinations where at least one data variable
-    # is not given:
-    for variable in product([None, data[:, 0]], repeat=3):
-        # Filter one valid configuration:
-        if not any(item is None for item in variable):
-            continue
-        with pytest.raises(GMTInvalidInput):
-            fig.contour(
-                x=variable[0],
-                y=variable[1],
-                z=variable[2],
-                region=region,
-                projection="X4i",
-                color="red",
-                frame="afg",
-                pen="",
-            )
-    # Should also fail if given too much data
-    with pytest.raises(GMTInvalidInput):
-        fig.contour(
-            x=data[:, 0],
-            y=data[:, 1],
-            z=data[:, 2],
-            data=data,
-            region=region,
-            projection="X10c",
-            style="c0.2c",
-            color="red",
-            frame="afg",
-            pen=True,
-        )
-
-
 @pytest.mark.mpl_image_compare
 def test_contour_vec(region):
     """
diff --git a/pygmt/tests/test_plot3d.py b/pygmt/tests/test_plot3d.py
index 3eb39e3f..302edd23 100644
--- a/pygmt/tests/test_plot3d.py
+++ b/pygmt/tests/test_plot3d.py
@@ -68,48 +68,6 @@ def test_plot3d_red_circles_zsize(data, region):
     return fig
 
 
-def test_plot3d_fail_no_data(data, region):
-    """
-    Plot should raise an exception if no data is given.
-    """
-    fig = Figure()
-    with pytest.raises(GMTInvalidInput):
-        fig.plot3d(
-            region=region, projection="X10c", style="c0.2c", color="red", frame="afg"
-        )
-    with pytest.raises(GMTInvalidInput):
-        fig.plot3d(
-            x=data[:, 0],
-            region=region,
-            projection="X10c",
-            style="c0.2c",
-            color="red",
-            frame="afg",
-        )
-    with pytest.raises(GMTInvalidInput):
-        fig.plot3d(
-            y=data[:, 0],
-            region=region,
-            projection="X10c",
-            style="c0.2c",
-            color="red",
-            frame="afg",
-        )
-    # Should also fail if given too much data
-    with pytest.raises(GMTInvalidInput):
-        fig.plot3d(
-            x=data[:, 0],
-            y=data[:, 1],
-            z=data[:, 2],
-            data=data,
-            region=region,
-            projection="X10c",
-            style="c0.2c",
-            color="red",
-            frame="afg",
-        )
-
-
 def test_plot3d_fail_1d_array_with_data(data, region):
     """
     Should raise an exception if array color, size, intensity and transparency
diff --git a/pygmt/tests/test_surface.py b/pygmt/tests/test_surface.py
index 8d9d84bc..b5e93d50 100644
--- a/pygmt/tests/test_surface.py
+++ b/pygmt/tests/test_surface.py
@@ -59,19 +59,6 @@ def test_surface_input_xyz(ship_data):
     return output
 
 
-def test_surface_input_xy_no_z(ship_data):
-    """
-    Run surface by passing in x and y, but no z.
-    """
-    with pytest.raises(GMTInvalidInput):
-        surface(
-            x=ship_data.longitude,
-            y=ship_data.latitude,
-            spacing="5m",
-            region=[245, 255, 20, 30],
-        )
-
-
 def test_surface_wrong_kind_of_input(ship_data):
     """
     Run surface using grid input that is not file/matrix/vectors.