which(), image() and legend() don't support pathlib.Path inputs

[seisman]

I'm trying to fix a pathlib bug of plot and plot3d in #1831 and find that which(), image() and legend also don't support pathlib.Path inputs.

To reproduce the issue, run:

>>> import pygmt
>>> from pathlib import Path

>>> pygmt.which(Path("abc.txt"))

>>> fig = pygmt.Figure()
>>> fig.image(Path("abc.png"))

>>> fig.legend(spec=Path("legend.txt"))

The error message is: TypeError: sequence item 0: expected str instance, PosixPath found.

Other functions work because their input file is wrapped in a context manager, like

pygmt/pygmt/src/histogram.py

Lines 150 to 152 in 8fe2d33

	with file_context as infile:
	arg_str = " ".join([infile, build_arg_string(kwargs)])
	lib.call_module("histogram", arg_str)

To fix the bug, the simplest solution is changing input file to a string type, for example, changing fname to str(fname). Do anyone have better and more general solutions?

[seisman]

In pygmt.which() or other functions, we build the argument string using the following code:

pygmt/pygmt/src/which.py

Line 63 in 813142b

arg_str = " ".join([fname, build_arg_string(kwargs), "->" + tmpfile.name])

Perhaps we should refactor the build_arg_string function to:

build_arg_string(kwargs, infile=None, outfile=None)

and in build_arg_string, we always make sure that infile and outfile are converted to string type.

[seisman]

Does anyone have comments on the proposed refactor in #1837?

[maxrjones]

Does anyone have comments on the proposed refactor in #1837?

I like the idea in general. I would rather that all modules handle input and output using the same strategy rather than only applying this code structure to these three methods/functions. Would you be open to a larger refactor using the new build_arg_string input and output arguments in other functions/methods for consistency?

A couple other questions:

1. Should we do any type checking before converting to a string?
2. The only reference pathlib.Path objects in our docs are the changelog, virtualfile_from_data, and load_dataarray. Should we add this as an accepted input if it is something that we intend to support (as suggested by this issue)? This is something that is explicitly stated in the xarray and pandas docs when available as an input type (e.g., https://xarray.pydata.org/en/stable/generated/xarray.open_dataset.html#xarray.open_dataset and https://pandas.pydata.org/pandas-docs/dev/reference/api/pandas.read_csv.html)

[seisman]

Does anyone have comments on the proposed refactor in #1837?

I like the idea in general. I would rather that all modules handle input and output using the same strategy rather than only applying this code structure to these three methods/functions. Would you be open to a larger refactor using the new build_arg_string input and output arguments in other functions/methods for consistency?

Yes, the plan is to refactor build_arg_string in all functions.

1. Should we do any type checking before converting to a string?

Perhaps not necessary now. From what I see, infile/outfile is either a string, a pathlib.Path object or a virtual file (also a string), so converting everything to a string looks OK.

2. The only reference pathlib.Path objects in our docs are the changelog, virtualfile_from_data, and load_dataarray. Should we add this as an accepted input if it is something that we intend to support (as suggested by this issue)? This is something that is explicitly stated in the xarray and pandas docs when available as an input type (e.g., xarray.pydata.org/en/stable/generated/xarray.open_dataset.html#xarray.open_dataset and pandas.pydata.org/pandas-docs/dev/reference/api/pandas.read_csv.html)

Mention pathlib.Path in docstrings sounds a good idea. BTW, the planned tutorial at #1268 will also mention pathlib.Path support.

[seisman]

Closed by #1837.