Return xarray.DataArray if outgrid is None

[maxrjones]

Description of the desired feature

For the triangulate draft and grdhisteq, we return an xarray.DataArray if outgrid is True or None, but for other functions we require outgrid to be None to return an xarray.DataArray. Here is an example (requires gridding/triangulate branch):

import pygmt
grid = pygmt.triangulate.regular_grid(
    data="@Table_5_11.txt",
    region=[-0.2,  6.6, -0.2, 6.6],
    spacing=0.2,
    outgrid=True
) # Works
filtered_grid = pygmt.grdfilter(
    grid,
    distance=0,
    filter="c1",
    outgrid=True
) # Fails with "grdfilter [ERROR]: No filename provided"

I would prefer that we return an xarray.DataArray when outgrid is True or None, but otherwise grdhisteq and triangulate should be revised for consistency if we chose to require outgrid to be unset.

[weiji14]

Maybe just stick with None or str to be consistent, I'll modify #731 so that outgrid=True won't work. Edit: done at 39c85b0.

[maxrjones]

Maybe just stick with None or str to be consistent, I'll modify #731 so that outgrid=True won't work.

Actually outgrid=None does not work either, it needs to be unset:

grid = pygmt.datasets.load_earth_relief()
pygmt.grdcut(grid=grid, outgrid=None, region=[0, 2, 0, 2]) # grdcut [ERROR]: Option -G: Must specify output grid file

I think it's unusual to define the behavior based on whether the parameter is set/unset rather than the specific setting.

[weiji14]

Maybe just stick with None or str to be consistent, I'll modify #731 so that outgrid=True won't work.

Actually outgrid=None does not work either, it needs to be unset:

grid = pygmt.datasets.load_earth_relief()
pygmt.grdcut(grid=grid, outgrid=None, region=[0, 2, 0, 2]) # grdcut [ERROR]: Option -G: Must specify output grid file

I think it's unusual to define the behavior based on whether the parameter is set/unset rather than the specific setting.

This explicit setting of outgrid=None problem is similar to what @seisman discovered at #1665 (comment) with frame=None. Something to fix in the @use_alias decorator.

[weiji14]

Should the behaviour of outgrid=False be the same as outgrid=None? I.e. return an output as an xarray.DataArray?

Actually, let me rephrase that, if a user sets outgrid=False, should they be seeing an error like this?

        if status != 0:
>           raise GMTCLibError(
                f"Module '{module}' failed with status code {status}:\n{self._error_message}"
            )
E           pygmt.exceptions.GMTCLibError: Module 'grdgradient' failed with status code 72:
E           grdgradient [ERROR]: Option -G: Must specify output file

pygmt/clib/session.py:502: GMTCLibError

Answer: 1) Yes they deserve to see this error message. 2) No, they should see a nice error message. 3) outgrid=False should just mean return xarray.DataArray.

[maxrjones]

I think (2) because that error message is difficult to understand for non-GMT users and IMO it's not intuitive that outgrid=False would return a xarray.DataArray (e.g., when I first wrapped grdhisteq I added support for outgrid=True to return an xarray.DataArray)

[weiji14]

Ok, I'm leaning towards (1) (current behaviour) because the docstring says:

    ret: xarray.DataArray or None
        Return type depends on whether the ``outgrid`` parameter is set:

        - :class:`xarray.DataArray` if ``outgrid`` is not set
        - None if ``outgrid`` is set (grid output will be stored in file set by
          ``outgrid``)

So a boolean True/False shouldn't be allowed. This keeps the implementation simple (specifically, less if-then checks), and the GMTCLibError could be explained away as a bad case of #256 🙂

But let's get a third opinion on this, @seisman?

[seisman]

I also agree with (1) to make things simple. As for (2), we may need a more general solution to replace GMT options (e.g., -G) with PyGMT keywords (e.g., outgrid), but it should be done in a separate PR.