Standardize parameter name for table data inputs #1479

weiji14 · 2021-09-01T22:24:38Z

Description of the desired feature

Originally posted by @meghanrjones in #1441 (comment)

This is somewhat related - I am not sure if it's worth a deprecation but I think we should decide on whether the parameter for tabular input is data or table for all modules moving forward. Currently, this is the distribution:

data parameter: surface, wiggle, velo, rose, plot3d, plot, contour
table parameter: blockmedian, blockmean, histogram
other: grdtrack (points), text (textfiles)

It would also be worthwhile to set a consistent order for the positional arguments x, y, [z], data, [outfile]

Are you willing to help implement and maintain this feature? Discuss first

The text was updated successfully, but these errors were encountered:

weiji14 · 2021-09-01T23:12:21Z

One option to reduce the amount of deprecation notices is to keep two parameters, but with this rule:

data - use for the fig.* plotting functions (e.g. fig.plot, fig.contour, etc)
table - use for the data processing functions (e.g. blockmean, histogram, etc)

This way, we'll only need to do the renaming for surface ('data' -> 'table'), grdtrack ('points' -> 'table') and text ('textfiles' -> 'data'). I have some reservations with renaming text's 'textfiles' to 'data' since it's a bit non-standard (and need to wait for #1121), but otherwise, what do people think?

maxrjones · 2021-09-01T23:34:45Z

One option to reduce the amount of deprecation notices is to keep two parameters, but with this rule:
* `data` - use for the `fig.*` plotting functions (e.g. `fig.plot`, `fig.contour`, etc)

* `table` - use for the data processing functions (e.g. `blockmean`, `histogram`, etc)
This way, we'll only need to do the renaming for surface ('data' -> 'table'), grdtrack ('points' -> 'table') and text ('textfiles' -> 'data'). I have some reservations with renaming text's 'textfiles' to 'data' since it's a bit non-standard (and need to wait for #1121), but otherwise, what do people think?

One complication is that many modules can be plotting or processing (e.g., histogram is mostly plotting but can return statistics with -I). My preference would be to use data moving forward and, if needed, deprecate table to data in blockmean, blockmedian, and histogram. grdtrack and text are non-standard enough for different names in my opinion.

weiji14 · 2021-09-03T02:24:19Z

One complication is that many modules can be plotting or processing (e.g., histogram is mostly plotting but can return statistics with -I).

Those types of dual-purpose functions (e.g. histogram, rose, solar) might end up having two APIs, depending on how we decide at #896. E.g. a fig.histogram for plotting and pygmt.histogram for extracting data.

My preference would be to use data moving forward and, if needed, deprecate table to data in blockmean, blockmedian, and histogram. grdtrack and text are non-standard enough for different names in my opinion.

Ok, sounds like we need a vote. So the options are:

🎉 Use data only for all functions, as per Meghan at Standardize parameter name for table data inputs #1479 (comment)
🚀 Use table only for all functions
👀 Use data for plotting functions, table for processing functions as per Wei Ji at Standardize parameter name for table data inputs #1479 (comment).
😕 Some other alternative - leave a comment below.

Place your vote on this comment. Only one vote per person 😛

maxrjones · 2021-09-11T19:07:12Z

Can we also set the conventional order for the positional arguments as data/table, x, y, [z] or x, y, [z], data/table? My opinion is that this isn't worth deprecating any current syntax, but that #1434, #1429, #1418, #1379, #731 and #1491 should have the same order (if both data/table and x/y[/z] will be supported).

Here's the current count:

x, y, [z], data/table - (5 current functions) contour, plot3d, wiggle, surface, plot (Used in PRs Wrap nearneighbor #1379
data/table, [outfile], x, y, [z] - (2 current functions) blockmean, blockmedian (Used in PRs Wrap triangulate #731, Wrap blockmode #1456)
only data/table with no x/y[/z] option - (2 current functions and 2 functions in v0.5.0) histogram, sphdistance, xyz2grd, info (Used in PRs Wrap gmtselect #1429, Wrap sph2grd #1434, Wrap sphinterpolate #1418, Expand table-like input options for sphdistance #1491)

weiji14 · 2021-09-15T02:00:09Z

Can we also set the conventional order for the positional arguments as data/table, x, y, [z] or x, y, [z], data/table?

Yes, good point. I would say just go with data, x, y, [z], so people can do pygmt.somefunction(data) if data is a pandas.DataFrame/geopandas.GeoDataFrame or any table-like input, similar to other Python-like plotting/processing code like plt.imshow. In this case, we would need to modify:

x, y, [z], data/table - (5 current functions) contour, plot3d, wiggle, surface, plot (Used in PRs Wrap nearneighbor #1379

Debating on whether we should go through a deprecation cycle (2 releases) for the x/y/z/data -> data/x/y/z parameter ordering change. Personally I try to always do parameter=arg in my code, but many people just pass in the arguments directly. So maybe best to raise a warning to give people time to switch over 🙃

maxrjones · 2021-09-15T19:36:43Z

Can we also set the conventional order for the positional arguments as data/table, x, y, [z] or x, y, [z], data/table?

Yes, good point. I would say just go with data, x, y, [z], so people can do pygmt.somefunction(data) if data is a pandas.DataFrame/geopandas.GeoDataFrame or any table-like input, similar to other Python-like plotting/processing code like plt.imshow. In this case, we would need to modify:

x, y, [z], data/table - (5 current functions) contour, plot3d, wiggle, surface, plot (Used in PRs Wrap nearneighbor #1379

Debating on whether we should go through a deprecation cycle (2 releases) for the x/y/z/data -> data/x/y/z parameter ordering change. Personally I try to always do parameter=arg in my code, but many people just pass in the arguments directly. So maybe best to raise a warning to give people time to switch over 🙃

Sounds good. If we will make the change for the functions that have x/y/z/data, I think we should raise a warning.

seisman · 2021-09-22T10:58:37Z

Just want to confirm that we have reached a consensus that we will use data and the order data, x, y, z for table inputs. If yes, then we can start to update these functions.

If we will make the change for the functions that have x/y/z/data, I think we should raise a warning.

It's unclear to me how to raise a warning for this case. For users who do x=x, y=y, z=z, the parameter order doesn't matter, thus the warning is unnecessary and may be confusing. For users who do x, y, z, how can know they pass arguments directly without giving parameters?

maxrjones · 2021-09-22T22:29:06Z

Just want to confirm that we have reached a consensus that we will use data and the order data, x, y, z for table inputs. If yes, then we can start to update these functions.

Yes, I think so.

If we will make the change for the functions that have x/y/z/data, I think we should raise a warning.

It's unclear to me how to raise a warning for this case. For users who do x=x, y=y, z=z, the parameter order doesn't matter, thus the warning is unnecessary and may be confusing. For users who do x, y, z, how can know they pass arguments directly without giving parameters?

If the user passes in x, y, z, then len(args) will be at least 3. If the user passes in x=x, y=y, z=y, then len(args) will be 0, using a decorator similar to deprecate_parameter.

seisman · 2021-09-23T02:42:39Z

seisman · 2021-09-24T03:51:50Z

If the user passes in x, y, z, then len(args) will be at least 3. If the user passes in x=x, y=y, z=y, then len(args) will be 0, using a decorator similar to deprecate_parameter.

It looks like a good solution. I'll work on it.

seisman · 2021-09-27T12:07:30Z

If the user passes in x, y, z, then len(args) will be at least 3. If the user passes in x=x, y=y, z=y, then len(args) will be 0, using a decorator similar to deprecate_parameter.

I'm rethinking the decorator implemented in #1541.

We're changing the parameter order from x, y, [z], data to data, x, y, [z]. Before making the change, users can pass positional parameters x, y, **kwargs or x, y, z, **kwargs. After making the change, users still can pass positional parameters, but they can only use it in this way data, **kwargs.

Thus, actually, we should raise the warning only if len(args) > 1 (i.e., x, y or x, y, z), for non-plotting functions.

For plotting functions, there is always the self parameter, thus, we should check if len(args) > 2 instead.

maxrjones · 2021-09-27T13:50:18Z

If the user passes in x, y, z, then len(args) will be at least 3. If the user passes in x=x, y=y, z=y, then len(args) will be 0, using a decorator similar to deprecate_parameter.

I'm rethinking the decorator implemented in #1541.

We're changing the parameter order from x, y, [z], data to data, x, y, [z]. Before making the change, users can pass positional parameters x, y, **kwargs or x, y, z, **kwargs. After making the change, users still can pass positional parameters, but they can only use it in this way data, **kwargs.

Thus, actually, we should raise the warning only if len(args) > 1 (i.e., x, y or x, y, z), for non-plotting functions.

For plotting functions, there is always the self parameter, thus, we should check if len(args) > 2 instead.

In addition to len(args), we might actually want to check the type or shape of args[0] (for processing) or args[1] (for plotting). For example, in #1546 someone could pass in data, length, azimuth as positional arguments and should not get a warning even though len(args>2).

seisman · 2021-09-27T14:05:45Z

For example, in #1546 someone could pass in data, length, azimuth as positional arguments and should not get a warning even though len(args>2).

Users can pass data, data=data, or length=length, azimuth=azimuth, but they can't do data, length, azimuth because we have the data_kind function to make sure that users cannot pass both data and x,y.

maxrjones · 2021-09-27T14:29:03Z

For example, in #1546 someone could pass in data, length, azimuth as positional arguments and should not get a warning even though len(args>2).

Users can pass data, data=data, or length=length, azimuth=azimuth, but they can't do data, length, azimuth because we have the data_kind function to make sure that users cannot pass both data and x,y.

OK, that was a bad example. An alternate example for plot: data, None, None, size. I don't know why anyone would want to do this, but they could.

seisman · 2021-09-28T01:48:47Z

We're changing the parameter order from x, y, [z], data to data, x, y, [z]. Before making the change, users can pass positional parameters x, y, **kwargs or x, y, z, **kwargs. After making the change, users still can pass positional parameters, but they can only use it in this way data, **kwargs.

Thus, actually, we should raise the warning only if len(args) > 1 (i.e., x, y or x, y, z), for non-plotting functions.

For plotting functions, there is always the self parameter, thus, we should check if len(args) > 2 instead.

I've implemented it in 95fa173 as part of PR #1548.

For example, in #1546 someone could pass in data, length, azimuth as positional arguments and should not get a warning even though len(args>2).

Users can pass data, data=data, or length=length, azimuth=azimuth, but they can't do data, length, azimuth because we have the data_kind function to make sure that users cannot pass both data and x,y.

OK, that was a bad example. An alternate example for plot: data, None, None, size. I don't know why anyone would want to do this, but they could.

I prefer NOT to check these complicated cases:

Although users can pass data, None, None, size, I believe it's very rare. The current decorator only raise a warning, so users script won't stop working.
The decorator is temporary, and likely will be removed after v0.7.0. It's six months if we strictly following the 3-month release cycle.

seisman · 2021-10-03T12:53:22Z

After #1565 is merged, all functions will use consistent parameter name data, and consistent parameter order data, x, y, [z]. There are few exceptions (see https://www.pygmt.org/dev/api/index.html):

image(self, imagefile, ...)
legend(self, spec=None, ...)
meca(self, spec, scale, ...)
rose(data=None, length=None, azimuth=None, ...)
text(textfiles=None, x=None, y=None, ...)
grdtrack(points, grid, ...)
x2sys_cross(tracks=None, outfile=None, ...)

I think these exceptions are reasonable and don't need more changes. Thus, we can close the issue after #1565 is merged. Agree?

As per #1479.

Wrapping the triangulate function which does "Delaunay triangulation or Voronoi partitioning and gridding of Cartesian data". Original GMT documentation can be found at https://docs.generic-mapping-tools.org/6.3/triangulate.html. Aliased outgrid (G), spacing (I), projection (J), region (R), verbose (V), registration (r). * Refactor triangulate to use virtualfile_from_data * Refactor triangulate implementation to use pygmt.io.load_dataarray * Alias binary(b), nodata(d), find(e), coltypes(f), header(h), incols(i) * Rename the parameter 'table' to 'data' As per #1479. * Refactor test_triangulate to use Table_5_11_mean.xyz instead of tut_ship * Refactor test_triangulate_with_outgrid to use xr.testing.assert_allclose * Refactor test_triangulate_input_xyz to use pd.testing.assert_frame_equal * Implement regular_grid and delaunay_triples staticmethod for triangulate * Let list inputs to spacing (I) and incols (i) work Use I="sequence" and i="sequence_comma". * Ensure triangulate.delaunay_triples output_type is valid Must be either one of numpy, pandas or file * Autocorrect output_type to 'file' if outfile parameter is set * Allow only str or None inputs to outgrid parameter Xref #1807 * Use gmt get GMT_TRIANGULATE to check whether Watson or Shewchuk is used * State that Shewchuk is the default triangulation algorithm As per GenericMappingTools/gmt#6438 * Actually document the output_type parameter for delaunay_triples Co-authored-by: Will Schlitzer <[email protected]> Co-authored-by: Meghan Jones <[email protected]> Co-authored-by: Dongdong Tian <[email protected]>

Wrapping the triangulate function which does "Delaunay triangulation or Voronoi partitioning and gridding of Cartesian data". Original GMT documentation can be found at https://docs.generic-mapping-tools.org/6.3/triangulate.html. Aliased outgrid (G), spacing (I), projection (J), region (R), verbose (V), registration (r). * Refactor triangulate to use virtualfile_from_data * Refactor triangulate implementation to use pygmt.io.load_dataarray * Alias binary(b), nodata(d), find(e), coltypes(f), header(h), incols(i) * Rename the parameter 'table' to 'data' As per GenericMappingTools#1479. * Refactor test_triangulate to use Table_5_11_mean.xyz instead of tut_ship * Refactor test_triangulate_with_outgrid to use xr.testing.assert_allclose * Refactor test_triangulate_input_xyz to use pd.testing.assert_frame_equal * Implement regular_grid and delaunay_triples staticmethod for triangulate * Let list inputs to spacing (I) and incols (i) work Use I="sequence" and i="sequence_comma". * Ensure triangulate.delaunay_triples output_type is valid Must be either one of numpy, pandas or file * Autocorrect output_type to 'file' if outfile parameter is set * Allow only str or None inputs to outgrid parameter Xref GenericMappingTools#1807 * Use gmt get GMT_TRIANGULATE to check whether Watson or Shewchuk is used * State that Shewchuk is the default triangulation algorithm As per GenericMappingTools/gmt#6438 * Actually document the output_type parameter for delaunay_triples Co-authored-by: Will Schlitzer <[email protected]> Co-authored-by: Meghan Jones <[email protected]> Co-authored-by: Dongdong Tian <[email protected]>

weiji14 added question Further information is requested deprecation Deprecating a feature labels Sep 1, 2021

weiji14 mentioned this issue Sep 1, 2021

Rename outfile parameter in surface to be outgrid #1441

Closed

weiji14 mentioned this issue Sep 3, 2021

Wrap triangulate #731

Merged

10 tasks

weiji14 pinned this issue Sep 10, 2021

This was referenced Sep 10, 2021

Wrap sphdistance #1383

Merged

Wrap sph2grd #1434

Merged

weiji14 mentioned this issue Sep 11, 2021

Expand table-like input options for sphdistance #1491

Merged

5 tasks

weiji14 mentioned this issue Sep 15, 2021

Wrap nearneighbor #1379

Merged

5 tasks

weiji14 mentioned this issue Sep 20, 2021

Wrap sphinterpolate #1418

Merged

5 tasks

This was referenced Sep 23, 2021

pygmt.info: Deprecate parameter "table" to "data" (remove in v0.7.0) #1538

Merged

Figure.histogram: Deprecate parameter "table" to "data" (remove in v0.7.0) #1540

Merged

This was referenced Sep 24, 2021

Add a decorator to raise a FutureWarning when positional arguments are passed for functions that changed parameter orders #1541

Merged

pygmt.xyz2grd: Rename the parameter 'table' to 'data' #1545

Merged

This was referenced Sep 26, 2021

Figure.rose: Reorder input parameters to 'data, length, azimuth' #1546

Merged

Figure.plot: Reorder input parameters to "data, x, y" #1547

Merged

seisman added this to the 0.5.0 milestone Sep 26, 2021

seisman mentioned this issue Sep 27, 2021

Figure.wiggle: Reorder input parameter to 'data, x, y, z' #1548

Merged

5 tasks

This was referenced Oct 2, 2021

Figure.plot3d: Reorder input parameters to 'data, x, y, z' #1560

Merged

Figure.contour: Reorder input parameters to 'data, x, y, z' #1561

Merged

pygmt.surface: Reorder input parameters to 'data, x, y, z' #1562

Merged

seisman mentioned this issue Oct 3, 2021

pygmt.blockm*: Reorder input parameters to 'data, x, y, z' #1565

Merged

5 tasks

seisman added deprecation Deprecating a feature enhancement Improving an existing feature and removed question Further information is requested deprecation Deprecating a feature labels Oct 3, 2021

seisman closed this as completed in #1565 Oct 4, 2021

seisman unpinned this issue Oct 4, 2021

seisman mentioned this issue Oct 4, 2021

Update parameter 'table' to 'data' for gallery examples #1567

Merged

5 tasks

weiji14 mentioned this issue Oct 19, 2021

PyGMT: A Python interface for the Generic Mapping Tools pyOpenSci/software-submission#43

Closed

9 tasks

seisman mentioned this issue Jan 3, 2022

Wrap ternary #1431

Merged

5 tasks

weiji14 added a commit that referenced this issue Mar 12, 2022

Rename the parameter 'table' to 'data'

d00d347

As per #1479.

weiji14 mentioned this issue Apr 6, 2022

grdtrack not working with "profile" option #1827

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardize parameter name for table data inputs #1479

Standardize parameter name for table data inputs #1479

weiji14 commented Sep 1, 2021

weiji14 commented Sep 1, 2021

maxrjones commented Sep 1, 2021

weiji14 commented Sep 3, 2021

maxrjones commented Sep 11, 2021

weiji14 commented Sep 15, 2021

maxrjones commented Sep 15, 2021

seisman commented Sep 22, 2021

maxrjones commented Sep 22, 2021

seisman commented Sep 23, 2021 •

edited by weiji14

Loading

seisman commented Sep 24, 2021

seisman commented Sep 27, 2021

maxrjones commented Sep 27, 2021

seisman commented Sep 27, 2021

maxrjones commented Sep 27, 2021

seisman commented Sep 28, 2021

seisman commented Oct 3, 2021

Standardize parameter name for table data inputs #1479

Standardize parameter name for table data inputs #1479

Comments

weiji14 commented Sep 1, 2021

weiji14 commented Sep 1, 2021

maxrjones commented Sep 1, 2021

weiji14 commented Sep 3, 2021

maxrjones commented Sep 11, 2021

weiji14 commented Sep 15, 2021

maxrjones commented Sep 15, 2021

seisman commented Sep 22, 2021

maxrjones commented Sep 22, 2021

seisman commented Sep 23, 2021 • edited by weiji14 Loading

seisman commented Sep 24, 2021

seisman commented Sep 27, 2021

maxrjones commented Sep 27, 2021

seisman commented Sep 27, 2021

maxrjones commented Sep 27, 2021

seisman commented Sep 28, 2021

seisman commented Oct 3, 2021

seisman commented Sep 23, 2021 •

edited by weiji14

Loading