search

GenericMappingTools / pygmt

A Python interface for the Generic Mapping Tools.
https://www.pygmt.org
BSD 3-Clause "New" or "Revised" License
758 stars 220 forks source link

Description for the "columns" arguments is missing #764

Closed michaelgrund closed 3 years ago

michaelgrund commented 3 years ago

Please describe what could be improved about this page or the typo/mistake that you found:

I would suggest to add a short note regarding the supported syntax for the columns option when using e.g. a data file as input, something like

I tried to fix that suggestion by my own following the instructions given in the Contributing Guidelines under Editing the Documentation, however, the corresponding .rst file is not available in the doc folder.

seisman commented 3 years ago

the corresponding .rst file is not available in the doc folder.

The documentation is automatically generated from the docstrings in the Python codes, see https://github.com/GenericMappingTools/pygmt/blob/master/pygmt/base_plotting.py#L649

I would suggest to add a short note regarding the supported syntax for the columns option when using e.g. a data file as input, something like

data (str or 2d array) – Either a data file name or a 2d numpy array with the tabular data. Use option columns (i) to choose which columns are x, y, color, and size, respectively (e.g. columns = [0, 1] if the x values are stored in the first column and y values in the second one, note: zero-based indexing is used).

The description of the columns argument is missing. I think we should add an entry for the "columns" argument (like what for already have for zvalue, intensity), instead of explaining columns in the description of the data argument.

michaelgrund commented 3 years ago

opened a pull request with the suggested changes (according to your comment @seisman ).

weiji14 commented 3 years ago

Reopening this because the "columns" description is missing for plot3d at https://www.pygmt.org/dev/api/generated/pygmt.Figure.plot3d.html.

I'm marking this as a good first issue as it should be easy enough to just copy and paste the "columns" description from https://github.com/GenericMappingTools/pygmt/blob/v0.3.0/pygmt/src/plot.py to https://github.com/GenericMappingTools/pygmt/blob/v0.3.0/pygmt/src/plot3d.py.

seisman commented 3 years ago

I'm not sure if we discussed it before (github search shows nothing). Is "columns" a good alias for -i option?

GMT provides both -i and -o, for selecting columns of input and output, respectively (see https://docs.generic-mapping-tools.org/latest/std-opts.html).

GMT uses incols and outcols for their long options (see https://github.com/GenericMappingTools/gmt/blob/master/src/gmt_common_longoptions.h).

Should we follow the GMT convention? If we choose columns for -i, what can we use for -o?

weiji14 commented 3 years ago

I think the comment you're looking for is this:

Have we used the columns alias for -i elsewhere? It may be a bad choice, because GMT also has -o which controls columns of output.

In https://github.com/GenericMappingTools/gmt/pull/230, GMT uses read-columns for -i and write-columns for -o.

GMT.jl uses incol for -i and outcol for -o.

_Originally posted by @seisman in https://github.com/GenericMappingTools/pygmt/pull/666#discussion_r511538159_

We kept columns as the alias for i because contour uses it too:

It is used in contour:

https://github.com/GenericMappingTools/pygmt/blob/d3e131a863a008f14979e7d9764cc6167156376f/pygmt/base_plotting.py#L576

I can take this out though, since it's not as useful in the Python world where we can select columns using indexing.

_Originally posted by @weiji14 in https://github.com/GenericMappingTools/pygmt/pull/666#discussion_r511538764_

Since renaming the alias for i will be a backwards incompatible break, I would suggest holding #1040 off until v0.4.0 (or we can go ahead with it since the alias already exists, just that it's undocumented in the API docstring).

seisman commented 3 years ago

Great searching skills! 😃

Since renaming the alias for i will be a backwards incompatible break, I would suggest holding #1040 off until v0.4.0

Yes to me.

willschlitzer commented 3 years ago

Great searching skills! smiley

Since renaming the alias for i will be a backwards incompatible break, I would suggest holding #1040 off until v0.4.0

Yes to me.

Sounds good. I'll update the milestone on it.

seisman commented 3 years ago

Do we all agree that we should use incols rather than columns for -i? If so, we need to deprecate columns to incols using the deprecate_parameter decorator.

weiji14 commented 3 years ago

Do we all agree that we should use incols rather than columns for -i? If so, we need to deprecate columns to incols using the deprecate_parameter decorator.

incol or incols? GMT.jl allows for either I think, while GMT core uses incols. Didn't we have something about using singular instead of plural if possible?

maxrjones commented 3 years ago

Do we all agree that we should use incols rather than columns for -i? If so, we need to deprecate columns to incols using the deprecate_parameter decorator.

incol or incols? GMT.jl allows for either I think, while GMT core uses incols. Didn't we have something about using singular instead of plural if possible?

For reference, incols rather than incol in GMT core comes from the brief discussion in https://github.com/GenericMappingTools/gmt/pull/4634. There are a few common options where PyGMT conventions differ from the current GMT long options (e.g., nodata). I will work on the organization-wide repo for 'canonical' long options, in case we want to suggest revisions to the current versions in GMT since they are still generally undocumented. Is it decided that PyGMT will only support one alias for each GMT option (in contrast to GMT.jl's strategy), with the exception of aliases that are in the deprecation process?

weiji14 commented 3 years ago

Ok, let's go with incols then as per https://github.com/GenericMappingTools/gmt/pull/4634#issuecomment-757082092 and https://github.com/GenericMappingTools/pygmt/issues/764#issuecomment-834059812. Best to have PyGMT follow upstream GMT if a long option name already exists as in this case. I know Meghan started some work at https://github.com/GenericMappingTools/long-options and we should follow that thread closely for new alias naming conventions.