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

Complete documentation for plot #666

Merged
merged 9 commits into from
Oct 25, 2020
Merged

Complete documentation for plot #666

merged 9 commits into from
Oct 25, 2020

Conversation

weiji14
Copy link
Member

@weiji14 weiji14 commented Oct 23, 2020

Description of proposed changes

Considering that plot is one of the more widely used functions, we should document it better than what we currently have at https://www.pygmt.org/v0.2.0/api/generated/pygmt.Figure.plot.html! For reference, upstream GMT docs is at https://docs.generic-mapping-tools.org/6.1/plot.html and https://github.com/GenericMappingTools/gmt/blob/6.1/doc/rst/source/plot_common.rst_.

Live documentation preview is at https://pygmt-git-improve-docs-plot.gmt.vercel.app/api/generated/pygmt.Figure.plot.html#pygmt.Figure.plot.

TODO:

Arguments/Aliases to document (see also https://www.generic-mapping-tools.org/GMT.jl/latest/#GMT.plot-Tuple{Any}). New aliases are highlighted in bold:

Fixes #

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to docstrings or tutorials.

@weiji14 weiji14 added the documentation Improvements or additions to documentation label Oct 23, 2020
@weiji14 weiji14 added this to the 0.2.1 milestone Oct 23, 2020
@weiji14
Copy link
Member Author

weiji14 commented Oct 23, 2020

Comments/Suggestions on what to do for these?

  • F - connection/connectivity?
  • I - intens?
  • T - (do we need this?)
  • Z - value/level?

@seisman
Copy link
Member

seisman commented Oct 23, 2020

  • D - position: why position? The similar option is called offset in text().

  • E - error_bars: It's OK to add this option here, but note that the option means more columns in the input data, so currently it only works for a plaintext file or 2D numpy arrays.

  • F - connection/connectivity: both look good to me.

  • I - intens: intensity? Just like grdimage

  • T - (do we need this?) No to me.

  • Z - value/level? value is too general, and level is also a little confusing. What about zvalue?

@weiji14
Copy link
Member Author

weiji14 commented Oct 23, 2020

* `D - position`: why _position_? The similar option is called _offset_ in [`text()`](https://www.pygmt.org/dev/api/generated/pygmt.Figure.text.html#pygmt.Figure.text).

True, I'll change it to offset, was thinking of some other module like colorbar that uses D=position.

* `E - error_bars`: It's OK to add this option here, but note that the option means more columns in the input data, so currently it only works for a plaintext file or 2D numpy arrays.

Ok.

* `F - connection/connectivity`: both look good to me.

Let's go with connection to keep with GMT.jl

* `I - intens`: intensity? Just like **grdimage**

Ok, I prefer intensity too.

* `T - (do we need this?)` No to me.

Alright, will ignore this one

* `Z - value/level?` _value_ is too general, and _level_ is also a little confusing. What about _zvalue_?

With or without an s? zvalue/zvalues? We're using Z=trackvalues in x2sys_cross.

@seisman
Copy link
Member

seisman commented Oct 23, 2020

With or without an s? zvalue/zvalues? We're using Z=trackvalues in x2sys_cross.

In GMT CLI, it could be a single value or an input file that contains a list of values.

In PyGMT, I expect it to be a single value, a filename, or a list of values. So zvalues may make more sense. But, there are already some inconsistencies about plurals and singulars. For example:

  • In plot(), we use sizes (plural) and direction (singular), although they both accept 1D numpy arrays.
  • In text(), we use angle, justify and font (singluars), although they all accept a single value or an array.

@seisman
Copy link
Member

seisman commented Oct 23, 2020

I forgot to say that I prefer to use singulars if possible, e.g., use size instead of sizes in plot().

@weiji14
Copy link
Member Author

weiji14 commented Oct 23, 2020

Do we want to change sizes to size in this PR, and possibly break backward compatibility? There will be some tutorial examples we need to fix as well.

  • I - intens: intensity? Just like grdimage

Actually for grdimage, we use shading=I, not intensity. But shading doesn't quite make sense for vector features like points and lines.

@seisman
Copy link
Member

seisman commented Oct 24, 2020

Do we want to change sizes to size in this PR.

Yes to me. But we need to think more about how plot() method will be used as it's too complicated. GMT.jl implements some new methods, like arrow, bar, scatter, vlines, by wrapping the complicated plot module. We may need to think about if we want to do the similar high-level wrappers.

and possibly break backward compatibility? There will be some tutorial examples we need to fix as well.

Considering the long-term unsolved issue #256, perhaps we should try to keep backward compatibility, at least for the next few releases. Otherwise, users who use sizes would get an error message which is difficult to understand.

I - intens: intensity? Just like grdimage
Actually for grdimage, we use shading=I, not intensity. But shading doesn't quite make sense for vector features like points and lines.

Yes, intensity should be OK for plot().

@weiji14 weiji14 mentioned this pull request Oct 24, 2020
14 tasks
@weiji14
Copy link
Member Author

weiji14 commented Oct 24, 2020

Do we want to change sizes to size in this PR.

Yes to me. But we need to think more about how plot() method will be used as it's too complicated. GMT.jl implements some new methods, like arrow, bar, scatter, vlines, by wrapping the complicated plot module. We may need to think about if we want to do the similar high-level wrappers.

Ah, didn't realize GMT.jl has those! The high-level API is for another discussion, I know @leouieda was keen on it, and @MarkWieczorek's https://github.com/SHTOOLS/SHTOOLS library has actually done a lot around PyGMT as well. There's also some discussion over at https://forum.generic-mapping-tools.org/t/pygmt-as-a-high-level-api-for-geovisualization/590, and your #670 feature request is an interesting step in that direction.

and possibly break backward compatibility? There will be some tutorial examples we need to fix as well.

Considering the long-term unsolved issue #256, perhaps we should try to keep backward compatibility, at least for the next few releases. Otherwise, users who use sizes would get an error message which is difficult to understand.

Ok, will keep to using sizes for now, and break backward compatibility in PyGMT v0.3.0. Ideally we would have a deprecation warning set-up to be nice, but that will be more work for this PR 😆

Yes, intensity should be OK for plot().

👍

@weiji14 weiji14 marked this pull request as ready for review October 24, 2020 21:05
pygmt/base_plotting.py Outdated Show resolved Hide resolved
Co-Authored-By: Dongdong Tian <[email protected]>
@weiji14
Copy link
Member Author

weiji14 commented Oct 25, 2020

@seisman any other outstanding issues on this? I'd like to merge this in and work on the plot3d implementation (anticipating some tricky merge conflicts).

As agreed above, we'll figure out how to pass in an 'error' column with error_bars (E) and rename 'sizes' to 'size' in a separate PR next time.

pygmt/base_plotting.py Outdated Show resolved Hide resolved
pygmt/base_plotting.py Outdated Show resolved Hide resolved
pygmt/base_plotting.py Show resolved Hide resolved
pygmt/base_plotting.py Outdated Show resolved Hide resolved
Also remove columns (i) alias for now.

Co-Authored-By: Dongdong Tian <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants