Repository: GenericMappingTools/pygmt

Issue:
Figure.grdimage does not respect shading=None
**Description of the problem**

Figure.grdimage() crashes when specifying the option `shading=None`. According to the docstring, `shading=None` is the default value, so this is weird. For the script I am using, I need to always specify something for shading (either nothing, or a file), so this breaks everything for me. This appears to have been broken since version 0.4

**Full code that generated the error**

This works
```python
fig = pygmt.Figure()
fig.grdimage(grid="@earth_relief_01d_g")

fig = pygmt.Figure()
fig.grdimage(grid="@earth_relief_01d_g", shading="@earth_relief_01d_g+d")
```

This doesn't
```python
fig = pygmt.Figure()
fig.grdimage(grid="@earth_relief_01d_g", shading=None)
```
**Full error message**

```
---------------------------------------------------------------------------
GMTInvalidInput                           Traceback (most recent call last)
Input In [10], in <cell line: 4>()
      1 import pygmt
      3 fig = pygmt.Figure()
----> 4 fig.grdimage(grid="@earth_relief_01d_g", shading=None)

File /opt/miniconda3/envs/testing/lib/python3.9/site-packages/pygmt/helpers/decorators.py:585, in use_alias.<locals>.alias_decorator.<locals>.new_module(*args, **kwargs)
    580         msg = (
    581             f"Short-form parameter ({short_param}) is not recommended. "
    582             f"Use long-form parameter '{long_alias}' instead."
    583         )
    584         warnings.warn(msg, category=SyntaxWarning, stacklevel=2)
--> 585 return module_func(*args, **kwargs)

File /opt/miniconda3/envs/testing/lib/python3.9/site-packages/pygmt/helpers/decorators.py:725, in kwargs_to_strings.<locals>.converter.<locals>.new_module(*args, **kwargs)
    723             kwargs[arg] = separators[fmt].join(f"{item}" for item in value)
    724 # Execute the original function and return its output
--> 725 return module_func(*args, **kwargs)

File /opt/miniconda3/envs/testing/lib/python3.9/site-packages/pygmt/src/grdimage.py:169, in grdimage(self, grid, **kwargs)
    166 file_context = lib.virtualfile_from_data(check_kind="raster", data=grid)
    167 with contextlib.ExitStack() as stack:
    168     # shading using an xr.DataArray
--> 169     if "I" in kwargs and data_kind(kwargs["I"]) == "grid":
    170         shading_context = lib.virtualfile_from_grid(kwargs["I"])
    171         kwargs["I"] = stack.enter_context(shading_context)

File /opt/miniconda3/envs/testing/lib/python3.9/site-packages/pygmt/helpers/utils.py:69, in data_kind(data, x, y, z, required_z)
     19 """
     20 Check what kind of data is provided to a module.
     21
   (...)
     66 'grid'
     67 """
     68 if data is None and x is None and y is None:
---> 69     raise GMTInvalidInput("No input data provided.")
     70 if data is not None and (x is not None or y is not None or z is not None):
     71     raise GMTInvalidInput("Too much data. Use either data or x and y.")

GMTInvalidInput: No input data provided.
```

**System information**

macos and ubuntu.


Discussion:
weiji14: I think I'll stop here for now to keep this PR fairly 'small'. There's still a list of other functions that need to be refactored to use `.get()`. Search for `" not in kwargs` and you'll find this list:

- [ ] meca
- [ ] nearneighbor
- [ ] sphdistance
- [ ] sphinterpolate
- [ ] surface
- [ ] text
- [ ] xyz2grd

Would someone else be willing to open a Part 2 PR for this?
meghanrjones: > Would someone else be willing to open a Part 2 PR for this?

I could open part 2.


weiji14: > > Would someone else be willing to open a Part 2 PR for this?
> 
> I could open part 2.

Cool, I tried to use the walrus operator in this Part 1 PR but it didn't seem to work for the functions I was refactoring. Maybe you'll have better luck in Part 2 :wink:
meghanrjones: > > > Would someone else be willing to open a Part 2 PR for this?
> > 
> > 
> > I could open part 2.
> 
> Cool, I tried to use the walrus operator in this Part 1 PR but it didn't seem to work for the functions I was refactoring. Maybe you'll have better luck in Part 2 😉

Should I open the PR before or after this one is approved/merged?
weiji14: > > > > Would someone else be willing to open a Part 2 PR for this?
> > > 
> > > 
> > > I could open part 2.
> > 
> > 
> > Cool, I tried to use the walrus operator in this Part 1 PR but it didn't seem to work for the functions I was refactoring. Maybe you'll have better luck in Part 2 wink
> 
> Should I open the PR before or after this one is approved/merged?

You can open the Part 2 PR now if you want, I think there shouldn't be any conflicts. Probably better to work on both in parallel too in case we notice anything wrong with the implementation in one or the other.
weiji14: /format
meghanrjones: > Just to be safe, let's wait until the Part 2 PR is opened and finalized before merging this into the main branch. I'll keep this PR in draft for now.

Working on it now!
meghanrjones: I am leaving meca out of part 2, since it is undergoing a complete refactor in https://github.com/GenericMappingTools/pygmt/pull/1784.

Patch:
diff --git a/pygmt/src/grd2cpt.py b/pygmt/src/grd2cpt.py
index 3c20e8c8..13d45b90 100644
--- a/pygmt/src/grd2cpt.py
+++ b/pygmt/src/grd2cpt.py
@@ -160,14 +160,14 @@ def grd2cpt(grid, **kwargs):
         ``categorical=True``.
     {V}
     """
-    if "W" in kwargs and "Ww" in kwargs:
+    if kwargs.get("W") is not None and kwargs.get("Ww") is not None:
         raise GMTInvalidInput("Set only categorical or cyclic to True, not both.")
     with Session() as lib:
         file_context = lib.virtualfile_from_data(check_kind="raster", data=grid)
         with file_context as infile:
-            if "H" not in kwargs:  # if no output is set
+            if kwargs.get("H") is None:  # if no output is set
                 arg_str = build_arg_string(kwargs, infile=infile)
-            if "H" in kwargs:  # if output is set
+            else:  # if output is set
                 outfile, kwargs["H"] = kwargs["H"], True
                 if not outfile or not isinstance(outfile, str):
                     raise GMTInvalidInput("'output' should be a proper file name.")
diff --git a/pygmt/src/grd2xyz.py b/pygmt/src/grd2xyz.py
index 54f03fdc..bb347009 100644
--- a/pygmt/src/grd2xyz.py
+++ b/pygmt/src/grd2xyz.py
@@ -159,7 +159,7 @@ def grd2xyz(grid, output_type="pandas", outfile=None, **kwargs):
     elif outfile is None and output_type == "file":
         raise GMTInvalidInput("Must specify 'outfile' for ASCII output.")
 
-    if "o" in kwargs and output_type == "pandas":
+    if kwargs.get("o") is not None and output_type == "pandas":
         raise GMTInvalidInput(
             "If 'outcols' is specified, 'output_type' must be either 'numpy'"
             "or 'file'."
diff --git a/pygmt/src/grdgradient.py b/pygmt/src/grdgradient.py
index 70200d9b..98ada1ea 100644
--- a/pygmt/src/grdgradient.py
+++ b/pygmt/src/grdgradient.py
@@ -164,7 +164,7 @@ def grdgradient(grid, **kwargs):
     >>> new_grid = pygmt.grdgradient(grid=grid, azimuth=10)
     """
     with GMTTempFile(suffix=".nc") as tmpfile:
-        if "Q" in kwargs and "N" not in kwargs:
+        if kwargs.get("Q") is not None and kwargs.get("N") is None:
             raise GMTInvalidInput("""Must specify normalize if tiles is specified.""")
         if not args_in_kwargs(args=["A", "D", "E"], kwargs=kwargs):
             raise GMTInvalidInput(
@@ -174,9 +174,8 @@ def grdgradient(grid, **kwargs):
         with Session() as lib:
             file_context = lib.virtualfile_from_data(check_kind="raster", data=grid)
             with file_context as infile:
-                if "G" not in kwargs:  # if outgrid is unset, output to tempfile
-                    kwargs.update({"G": tmpfile.name})
-                outgrid = kwargs["G"]
+                if (outgrid := kwargs.get("G")) is None:
+                    kwargs["G"] = outgrid = tmpfile.name  # output to tmpfile
                 lib.call_module("grdgradient", build_arg_string(kwargs, infile=infile))
 
         return load_dataarray(outgrid) if outgrid == tmpfile.name else None
diff --git a/pygmt/src/grdimage.py b/pygmt/src/grdimage.py
index 8cc3d737..de130e70 100644
--- a/pygmt/src/grdimage.py
+++ b/pygmt/src/grdimage.py
@@ -166,7 +166,7 @@ def grdimage(self, grid, **kwargs):
         file_context = lib.virtualfile_from_data(check_kind="raster", data=grid)
         with contextlib.ExitStack() as stack:
             # shading using an xr.DataArray
-            if "I" in kwargs and data_kind(kwargs["I"]) == "grid":
+            if kwargs.get("I") is not None and data_kind(kwargs["I"]) == "grid":
                 shading_context = lib.virtualfile_from_grid(kwargs["I"])
                 kwargs["I"] = stack.enter_context(shading_context)
 
diff --git a/pygmt/src/grdview.py b/pygmt/src/grdview.py
index 0fbd8bca..03718807 100644
--- a/pygmt/src/grdview.py
+++ b/pygmt/src/grdview.py
@@ -126,7 +126,8 @@ def grdview(self, grid, **kwargs):
         file_context = lib.virtualfile_from_data(check_kind="raster", data=grid)
 
         with contextlib.ExitStack() as stack:
-            if "G" in kwargs:  # deal with kwargs["G"] if drapegrid is xr.DataArray
+            if kwargs.get("G") is not None:
+                # deal with kwargs["G"] if drapegrid is xr.DataArray
                 drapegrid = kwargs["G"]
                 if data_kind(drapegrid) in ("file", "grid"):
                     if data_kind(drapegrid) == "grid":
diff --git a/pygmt/src/makecpt.py b/pygmt/src/makecpt.py
index b0a8895d..bbb167dc 100644
--- a/pygmt/src/makecpt.py
+++ b/pygmt/src/makecpt.py
@@ -147,11 +147,11 @@ def makecpt(**kwargs):
         ``categorical=True``.
     """
     with Session() as lib:
-        if "W" in kwargs and "Ww" in kwargs:
+        if kwargs.get("W") is not None and kwargs.get("Ww") is not None:
             raise GMTInvalidInput("Set only categorical or cyclic to True, not both.")
-        if "H" not in kwargs:  # if no output is set
+        if kwargs.get("H") is None:  # if no output is set
             arg_str = build_arg_string(kwargs)
-        elif "H" in kwargs:  # if output is set
+        else:  # if output is set
             outfile, kwargs["H"] = kwargs.pop("H"), True
             if not outfile or not isinstance(outfile, str):
                 raise GMTInvalidInput("'output' should be a proper file name.")
diff --git a/pygmt/src/plot.py b/pygmt/src/plot.py
index 82fd0187..a9a21fbb 100644
--- a/pygmt/src/plot.py
+++ b/pygmt/src/plot.py
@@ -218,15 +218,15 @@ def plot(self, data=None, x=None, y=None, size=None, direction=None, **kwargs):
     kind = data_kind(data, x, y)
 
     extra_arrays = []
-    if "S" in kwargs and kwargs["S"][0] in "vV" and direction is not None:
+    if kwargs.get("S") is not None and kwargs["S"][0] in "vV" and direction is not None:
         extra_arrays.extend(direction)
     elif (
-        "S" not in kwargs
+        kwargs.get("S") is None
         and kind == "geojson"
         and data.geom_type.isin(["Point", "MultiPoint"]).all()
     ):  # checking if the geometry of a geoDataFrame is Point or MultiPoint
         kwargs["S"] = "s0.2c"
-    elif "S" not in kwargs and kind == "file" and data.endswith(".gmt"):
+    elif kwargs.get("S") is None and kind == "file" and data.endswith(".gmt"):
         # checking that the data is a file path to set default style
         try:
             with open(which(data), mode="r", encoding="utf8") as file:
@@ -236,7 +236,7 @@ def plot(self, data=None, x=None, y=None, size=None, direction=None, **kwargs):
                 kwargs["S"] = "s0.2c"
         except FileNotFoundError:
             pass
-    if "G" in kwargs and is_nonstr_iter(kwargs["G"]):
+    if kwargs.get("G") is not None and is_nonstr_iter(kwargs["G"]):
         if kind != "vectors":
             raise GMTInvalidInput(
                 "Can't use arrays for color if data is matrix or file."
@@ -251,7 +251,7 @@ def plot(self, data=None, x=None, y=None, size=None, direction=None, **kwargs):
         extra_arrays.append(size)
 
     for flag in ["I", "t"]:
-        if flag in kwargs and is_nonstr_iter(kwargs[flag]):
+        if kwargs.get(flag) is not None and is_nonstr_iter(kwargs[flag]):
             if kind != "vectors":
                 raise GMTInvalidInput(
                     f"Can't use arrays for {plot.aliases[flag]} if data is matrix or file."
diff --git a/pygmt/src/plot3d.py b/pygmt/src/plot3d.py
index 504daf6b..adecd87a 100644
--- a/pygmt/src/plot3d.py
+++ b/pygmt/src/plot3d.py
@@ -188,15 +188,15 @@ def plot3d(
     kind = data_kind(data, x, y, z)
 
     extra_arrays = []
-    if "S" in kwargs and kwargs["S"][0] in "vV" and direction is not None:
+    if kwargs.get("S") is not None and kwargs["S"][0] in "vV" and direction is not None:
         extra_arrays.extend(direction)
     elif (
-        "S" not in kwargs
+        kwargs.get("S") is None
         and kind == "geojson"
         and data.geom_type.isin(["Point", "MultiPoint"]).all()
     ):  # checking if the geometry of a geoDataFrame is Point or MultiPoint
         kwargs["S"] = "u0.2c"
-    elif "S" not in kwargs and kind == "file" and data.endswith(".gmt"):
+    elif kwargs.get("S") is None and kind == "file" and data.endswith(".gmt"):
         # checking that the data is a file path to set default style
         try:
             with open(which(data), mode="r", encoding="utf8") as file:
@@ -206,7 +206,7 @@ def plot3d(
                 kwargs["S"] = "u0.2c"
         except FileNotFoundError:
             pass
-    if "G" in kwargs and is_nonstr_iter(kwargs["G"]):
+    if kwargs.get("G") is not None and is_nonstr_iter(kwargs["G"]):
         if kind != "vectors":
             raise GMTInvalidInput(
                 "Can't use arrays for color if data is matrix or file."
@@ -221,7 +221,7 @@ def plot3d(
         extra_arrays.append(size)
 
     for flag in ["I", "t"]:
-        if flag in kwargs and is_nonstr_iter(kwargs[flag]):
+        if kwargs.get(flag) is not None and is_nonstr_iter(kwargs[flag]):
             if kind != "vectors":
                 raise GMTInvalidInput(
                     f"Can't use arrays for {plot3d.aliases[flag]} if data is matrix or file."
diff --git a/pygmt/src/project.py b/pygmt/src/project.py
index 21a0d858..4715e7db 100644
--- a/pygmt/src/project.py
+++ b/pygmt/src/project.py
@@ -210,13 +210,13 @@ def project(data=None, x=None, y=None, z=None, outfile=None, **kwargs):
           by ``outfile``)
     """
 
-    if "C" not in kwargs:
+    if kwargs.get("C") is None:
         raise GMTInvalidInput("The `center` parameter must be specified.")
-    if "G" not in kwargs and data is None:
+    if kwargs.get("G") is None and data is None:
         raise GMTInvalidInput(
             "The `data` parameter must be specified unless `generate` is used."
         )
-    if "G" in kwargs and "F" in kwargs:
+    if kwargs.get("G") is not None and kwargs.get("F") is not None:
         raise GMTInvalidInput(
             "The `convention` parameter is not allowed with `generate`."
         )
@@ -225,7 +225,7 @@ def project(data=None, x=None, y=None, z=None, outfile=None, **kwargs):
         if outfile is None:  # Output to tmpfile if outfile is not set
             outfile = tmpfile.name
         with Session() as lib:
-            if "G" not in kwargs:
+            if kwargs.get("G") is None:
                 # Choose how data will be passed into the module
                 table_context = lib.virtualfile_from_data(
                     check_kind="vector", data=data, x=x, y=y, z=z, required_z=False
@@ -240,7 +240,7 @@ def project(data=None, x=None, y=None, z=None, outfile=None, **kwargs):
 
         # if user did not set outfile, return pd.DataFrame
         if outfile == tmpfile.name:
-            if "G" in kwargs:
+            if kwargs.get("G") is not None:
                 column_names = list("rsp")
                 result = pd.read_csv(tmpfile.name, sep="\t", names=column_names)
             else:
diff --git a/pygmt/src/solar.py b/pygmt/src/solar.py
index 5b0c0a35..51c830d1 100644
--- a/pygmt/src/solar.py
+++ b/pygmt/src/solar.py
@@ -66,7 +66,7 @@ def solar(self, terminator="d", terminator_datetime=None, **kwargs):
     """
 
     kwargs = self._preprocess(**kwargs)  # pylint: disable=protected-access
-    if "T" in kwargs:
+    if kwargs.get("T") is not None:
         raise GMTInvalidInput(
             "Use 'terminator' and 'terminator_datetime' instead of 'T'."
         )
diff --git a/pygmt/src/text.py b/pygmt/src/text.py
index 2e81596c..d12ffcd9 100644
--- a/pygmt/src/text.py
+++ b/pygmt/src/text.py
@@ -215,7 +215,7 @@ def text_(
     extra_arrays = []
     # If an array of transparency is given, GMT will read it from
     # the last numerical column per data record.
-    if "t" in kwargs and is_nonstr_iter(kwargs["t"]):
+    if kwargs.get("t") is not None and is_nonstr_iter(kwargs["t"]):
         extra_arrays.append(kwargs["t"])
         kwargs["t"] = ""
 
diff --git a/pygmt/src/velo.py b/pygmt/src/velo.py
index 77d7115b..c62c46ff 100644
--- a/pygmt/src/velo.py
+++ b/pygmt/src/velo.py
@@ -238,8 +238,12 @@ def velo(self, data=None, **kwargs):
     """
     kwargs = self._preprocess(**kwargs)  # pylint: disable=protected-access
 
-    if "S" not in kwargs or ("S" in kwargs and not isinstance(kwargs["S"], str)):
-        raise GMTInvalidInput("Spec is a required argument and has to be a string.")
+    if kwargs.get("S") is None or (
+        kwargs.get("S") is not None and not isinstance(kwargs["S"], str)
+    ):
+        raise GMTInvalidInput(
+            "The parameter `spec` is required and has to be a string."
+        )
 
     if isinstance(data, np.ndarray) and not pd.api.types.is_numeric_dtype(data):
         raise GMTInvalidInput(


Test patch:
diff --git a/pygmt/tests/test_grd2xyz.py b/pygmt/tests/test_grd2xyz.py
index 97abb590..4cb172d3 100644
--- a/pygmt/tests/test_grd2xyz.py
+++ b/pygmt/tests/test_grd2xyz.py
@@ -45,7 +45,7 @@ def test_grd2xyz_format(grid):
     np.testing.assert_allclose(orig_val, xyz_val)
     xyz_array = grd2xyz(grid=grid, output_type="numpy")
     assert isinstance(xyz_array, np.ndarray)
-    xyz_df = grd2xyz(grid=grid, output_type="pandas")
+    xyz_df = grd2xyz(grid=grid, output_type="pandas", outcols=None)
     assert isinstance(xyz_df, pd.DataFrame)
     assert list(xyz_df.columns) == ["lon", "lat", "z"]
 
diff --git a/pygmt/tests/test_grdgradient.py b/pygmt/tests/test_grdgradient.py
index 4f908a22..df4810e2 100644
--- a/pygmt/tests/test_grdgradient.py
+++ b/pygmt/tests/test_grdgradient.py
@@ -57,8 +57,13 @@ def test_grdgradient_no_outgrid(grid, expected_grid):
     """
     Test the azimuth and direction parameters for grdgradient with no set
     outgrid.
+
+    This is a regression test for
+    https://github.com/GenericMappingTools/pygmt/issues/1807.
     """
-    result = grdgradient(grid=grid, azimuth=10, region=[-53, -49, -20, -17])
+    result = grdgradient(
+        grid=grid, azimuth=10, region=[-53, -49, -20, -17], outgrid=None
+    )
     # check information of the output grid
     assert isinstance(result, xr.DataArray)
     assert result.gmt.gtype == 1  # Geographic grid
diff --git a/pygmt/tests/test_grdimage.py b/pygmt/tests/test_grdimage.py
index b6ec545b..efa1e73e 100644
--- a/pygmt/tests/test_grdimage.py
+++ b/pygmt/tests/test_grdimage.py
@@ -92,6 +92,21 @@ def test_grdimage_file():
     return fig
 
 
+@pytest.mark.mpl_image_compare(filename="test_grdimage_slice.png")
+@pytest.mark.parametrize("shading", [None, False])
+def test_grdimage_default_no_shading(grid, shading):
+    """
+    Plot an image with no shading.
+
+    This is a regression test for
+    https://github.com/GenericMappingTools/pygmt/issues/1852
+    """
+    grid_ = grid.sel(lat=slice(-30, 30))
+    fig = Figure()
+    fig.grdimage(grid_, cmap="earth", projection="M6i", shading=shading)
+    return fig
+
+
 @check_figures_equal()
 @pytest.mark.parametrize(
     "shading",
diff --git a/pygmt/tests/test_text.py b/pygmt/tests/test_text.py
index b0f5158e..c160b821 100644
--- a/pygmt/tests/test_text.py
+++ b/pygmt/tests/test_text.py
@@ -337,6 +337,20 @@ def test_text_varying_transparency():
     return fig
 
 
+@pytest.mark.mpl_image_compare(filename="test_text_input_single_filename.png")
+@pytest.mark.parametrize("transparency", [None, False, 0])
+def test_text_no_transparency(transparency):
+    """
+    Add text with no transparency set.
+
+    This is a regression test for
+    https://github.com/GenericMappingTools/pygmt/issues/1852.
+    """
+    fig = Figure()
+    fig.text(region=[10, 70, -5, 10], textfiles=POINTS_DATA, transparency=transparency)
+    return fig
+
+
 @pytest.mark.mpl_image_compare
 def test_text_nonstr_text():
     """