GenericMappingTools / GenericMappingTools/pygmt

Ideas on refactoring the Session.virtualfile_in method

Open
#3,836 0 comments 1 reaction 1 assignee Claimed by @seisman View on GitHub
maintenance
Dominant language
Python
Stars
874
Forks
255
Avg merge
1d 21h
Merged PRs (30d)
40

Description

The `Session.virtualfile_in` method is one of the most commonly used low-level API functions when wrapping GMT modules. Currently, its definition signature is as below:

https://github.com/GenericMappingTools/pygmt/blob/1791ca29547a41bef791be2df78c44620e327423/pygmt/clib/session.py#L1768-L1778

Here are some ideas to refactor the method signature:

1. Remove the `extra_arrays` parameter (Refer to
https://github.com/GenericMappingTools/pygmt/pull/3823#issue-2880404911 for more detailed reasoning). When a module takes more than 3 columns, we can build a dict of columns instead. This is currently WIP in #3823.
2. Remove `required_z` and add `required_ncols` (or `mincols` for a shorter name). In this way, we can check if a table input contains enough number of columns. `required_z=True` is equivalent to `required_ncols=3`. This is currently WIP in #3369
3. Rename `required_data` to `required`. After addressing points 1 and 2, the function definition will be:
```
def virtualfile_in(
self,
check_kind=None,
data=None,
x=None,
y=None,
z=None,
mincols=2,
required_data=True,
):
```
I think it makes more sense to rename `required_data` to `required`.
4. In some wrappers (currently, `plot`/`plot3d`/`text`/`legend`/`x2sys_cross`), we need to call `data_kind` to decide the data kind. In this way, we already know the data kind before calling `Session.virtualfile_in`, so it's unnecessary to call `data_kind` and check if the kind is valid in `Session.virtualfile_in`. So, what about renaming `check_kind` to `kind`? Currently, `check_kind` can take two values, `vector` and `raster`. After renaming, the new `kind` parameter can accept the following values, in addition to `"vector"` and `"raster"` https://github.com/GenericMappingTools/pygmt/blob/1791ca29547a41bef791be2df78c44620e327423/pygmt/helpers/utils.py#L267

In this way, `"vector"` and `"raster"` can be thought of as two special/general kinds (but please note that there may be confusions for the `vector` and `vectors` kinds). Then in the `Sesssion.virtualfile_in` method, the related codes can be rewritten to something like:
```
if kind in {"vector", "raster"}: # Two general kinds
valid_kinds = ("file", "arg") if required_data is False else ("file",)
if kind == "raster":
valid_kinds += ("grid", "image")
elif kind == "vector":
valid_kinds += ("empty", "matrix", "vectors", "geojson")
kind = data_kind(data) # Determine the actual data kind
if kind not in valid_kinds:
msg = f"Unrecognized data type for {check_kind}: {type(data)}."
raise GMTInvalidInput(msg)
```
5. Reorder the parameters
_Originally posted by @weiji14 in https://github.com/GenericMappingTools/pygmt/pull/3823#discussion_r1982214584_

> If we're going to break compatibility of `virtualfile_in` for 4 minor versions, maybe it's a good time to rethink the parameter ordering (ties in with potential changes in #3369). Would it make sense for the signature to be something like:
>
> ```
> def virtualfile_in(
> self,
> check_kind=None,
> required_data=True,
> required_z=False,
> data=None,
> x=None,
> y=None,
> z=None,
> **extra_arrays
> )
> ```
> ? We could also mark some of those parameters as keyword only (https://peps.python.org/pep-0570/#keyword-only-arguments) to ensure future compatibility (i.e. prevent positional-only parameters/arguments in case we decide to change the order again).

With points 1-4 addressed, maybe the following parameter order is better?
```
> def virtualfile_in(
> self,
> data=None,
> x=None,
> y=None,
> z=None,
> kind=None,
> required=True,
> mincols=2,
> )
```

## TODO

- [x] Remove the `extra_arrays` parameter #3823
- [x] Remove `required_z` and add `mincols` #3369
- [x] Rename `required_data` to `required` #3931
- [ ] Improve the `check_kind` parameter in `Session.virtualfile_in`
- [ ] Reorder the parameters in `Session.virtualfile_in`
- [ ] Refactor `validate_data_input`

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.