Description for the "columns" arguments is missing #764

Activity

1. seisman

   added

   documentationImprovements or additions to documentationImprovements or additions to documentation

   on Dec 25, 2020
2. seisman commented on Dec 25, 2020

   

   seisman

   on Dec 25, 2020

   Last edited by seisman

   Member

   More actions

   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.
3. michaelgrund commented on Dec 26, 2020

   

   michaelgrund

   on Dec 26, 2020

   MemberAuthor

   More actions

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

   changed the title [-]Suggested improvement for api/generated/pygmt.Figure.plot[/-] [+]Description for the "columns" arguments is missing[/+] on Dec 26, 2020
5. seisman

   closed this as completedin #766on Dec 27, 2020
6. weiji14 commented on Mar 4, 2021

   

   weiji14

   on Mar 4, 2021

   Last edited by weiji14

   Member

   More actions

   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.
7. weiji14

   reopened this on Mar 4, 2021
8. weiji14

   added

   good first issueGood for newcomersGood for newcomers

   on Mar 4, 2021
9. seisman commented on Mar 12, 2021

   

   seisman

   on Mar 12, 2021

   Member

   More actions

   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?
10. weiji14 commented on Mar 12, 2021

    

    weiji14

    on Mar 12, 2021

    Last edited by weiji14

    Member

    More actions

    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:

    pygmt/pygmt/base_plotting.py

    Line 576 in d3e131a

    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).
11. seisman commented on Mar 12, 2021

    

    seisman

    on Mar 12, 2021

    Member

    More actions

    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.
12. willschlitzer commented on Mar 12, 2021

    

    willschlitzer

    on Mar 12, 2021

    Contributor

    More actions

    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.
13. weiji14

    added

    deprecationDeprecating a featureDeprecating a feature

    on Mar 14, 2021
14. seisman commented on May 7, 2021

    

    seisman

    on May 7, 2021

    Member

    More actions

    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.
15. weiji14 commented on May 7, 2021

    

    weiji14

    on May 7, 2021

    Member

    More actions

    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?
16. maxrjones commented on May 7, 2021

    

    maxrjones

    on May 7, 2021

    Member

    More actions

    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?
17. weiji14 commented on May 26, 2021

    

    weiji14

    on May 26, 2021

    Member

    More actions

    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.
18. michaelgrund

    added a commit that references this issue on May 26, 2021

    Figure.plot: Deprecate parameter "columns" to "incols" (remove in v0.…

    ...

    0be992e
19. michaelgrund

    linked a pull request that will close this issueFigure.plot: Deprecate parameter "columns" to "incols" (remove in v0.6.0) #1298on May 26, 2021
20. seisman

    closed this as completedin #1298on May 28, 2021