Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Description for the "columns" arguments is missing #764

Closed
michaelgrund opened this issue Dec 25, 2020 · 11 comments · Fixed by #766, #1040 or #1298
Closed

Description for the "columns" arguments is missing #764

michaelgrund opened this issue Dec 25, 2020 · 11 comments · Fixed by #766, #1040 or #1298
Labels
deprecation Deprecating a feature documentation Improvements or additions to documentation good first issue Good for newcomers

Comments

@michaelgrund
Copy link
Member

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

  • 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).

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 seisman added the documentation Improvements or additions to documentation label Dec 25, 2020
@seisman
Copy link
Member

seisman commented Dec 25, 2020

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
Copy link
Member Author

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

@seisman seisman changed the title Suggested improvement for api/generated/pygmt.Figure.plot Description for the "columns" arguments is missing Dec 26, 2020
@weiji14
Copy link
Member

weiji14 commented Mar 4, 2021

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
Copy link
Member

seisman commented Mar 12, 2021

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
Copy link
Member

weiji14 commented Mar 12, 2021

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 GenericMappingTools/gmt#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 #666 (comment)

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

It is used in contour:

i="columns",

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 #666 (comment)

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
Copy link
Member

seisman commented Mar 12, 2021

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
Copy link
Contributor

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.

@weiji14 weiji14 added the deprecation Deprecating a feature label Mar 14, 2021
@weiji14 weiji14 mentioned this issue Mar 22, 2021
5 tasks
@seisman
Copy link
Member

seisman commented May 7, 2021

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
Copy link
Member

weiji14 commented May 7, 2021

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
Copy link
Member

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 GenericMappingTools/gmt#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
Copy link
Member

weiji14 commented May 26, 2021

Ok, let's go with incols then as per GenericMappingTools/gmt#4634 (comment) and #764 (comment). 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.

michaelgrund added a commit that referenced this issue May 26, 2021
…6.0)

As discussed in #764, this PR replaces the **columns** parameter by **incols** for plot.py and adds the deprecation .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation Deprecating a feature documentation Improvements or additions to documentation good first issue Good for newcomers
Projects
None yet
5 participants