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

Update the docstrings in the plotting modules #881

Merged
merged 59 commits into from
Feb 14, 2021

Conversation

maxrjones
Copy link
Member

@maxrjones maxrjones commented Feb 13, 2021

Description of proposed changes

Update the docstrings of aliases following the convention in #631.

To do:

  • Update docstrings for data modules
  • Format docstrings as raw strings

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.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

  • /format: automatically format and lint the code
  • /test-gmt-dev: run full tests on the latest GMT development version

@weiji14 weiji14 added the documentation Improvements or additions to documentation label Feb 13, 2021
@maxrjones
Copy link
Member Author

maxrjones commented Feb 13, 2021

I'm not sure how to mark this PR as 'draft' (edit: nevermind, found it). Currently done with the plotting modules and will update the data processing modules tomorrow.

@maxrjones maxrjones marked this pull request as draft February 13, 2021 04:36
@weiji14
Copy link
Member

weiji14 commented Feb 13, 2021

Wow, you're fast!

Currently done with the plotting modules and will update the data processing modules tomorrow.

To keep things small (and easier to review, see https://github.com/GenericMappingTools/pygmt/blob/master/CONTRIBUTING.md#general-guidelines), you can split it into 2 PRs, 1 for plotting modules (this PR), 1 for data processing modules. Just ask @willschlitzer to review if you think it's ready, I think he'll be keen and it's his ideal timezone now too.

@maxrjones
Copy link
Member Author

To keep things small (and easier to review, see https://github.com/GenericMappingTools/pygmt/blob/master/CONTRIBUTING.md#general-guidelines), you can split it into 2 PRs, 1 for plotting modules (this PR), 1 for data processing modules. Just ask @willschlitzer to review if you think it's ready, I think he'll be keen and it's his ideal timezone now too.

Sounds good. grd2cpt, makecpt, savefig, show, and psconvert also remain but those could similarly go in a different PR to keep things small. Also based on that recommendation, it seems that it's probably fine that this PR really just covers the simplest cases. An example of a more complex situation is meca, in which the arguments do not map simply to GMT options.

@maxrjones
Copy link
Member Author

Many of the arguments refer to other arguments, for example lakes from coast refers to water:

lakes : str or list
fill\ [+l|\ +r].
Set the shade, color, or pattern for lakes and river-lakes. The
default is the fill chosen for wet areas set by the water
argument. Optionally, specify separate fills by appending
+l for lakes or +r for river-lakes, and passing multiple
strings in a list.

Most of the references to other arguments are formatted in a code block, should this be kept or changed to bold similar to cross-references in gmt?

@seisman
Copy link
Member

seisman commented Feb 13, 2021

Many of the arguments refer to other arguments, for example lakes from coast refers to water:

lakes : str or list
fill\ [+l|\ +r].
Set the shade, color, or pattern for lakes and river-lakes. The
default is the fill chosen for wet areas set by the water
argument. Optionally, specify separate fills by appending
+l for lakes or +r for river-lakes, and passing multiple
strings in a list.

Most of the references to other arguments are formatted in a code block, should this be kept or changed to bold similar to cross-references in gmt?

IMHO, we should try our best to document the arguments of PyGMT modules, rather than refering users to read the GMT documentation. That's why I prefer to use code blocks for PyGMT arguments.

@seisman seisman added this to the 0.3.0 milestone Feb 13, 2021
pygmt/src/basemap.py Outdated Show resolved Hide resolved
pygmt/src/colorbar.py Outdated Show resolved Hide resolved
pygmt/src/colorbar.py Outdated Show resolved Hide resolved
pygmt/src/colorbar.py Outdated Show resolved Hide resolved
pygmt/src/coast.py Outdated Show resolved Hide resolved
pygmt/src/contour.py Outdated Show resolved Hide resolved
@willschlitzer
Copy link
Contributor

Nice work!

@willschlitzer willschlitzer changed the title WIP: Update the docstrings to match gmt docs WIP: Update the plotting function docstrings to match gmt docs Feb 13, 2021
@willschlitzer
Copy link
Contributor

Wow, you're fast!

Currently done with the plotting modules and will update the data processing modules tomorrow.

To keep things small (and easier to review, see https://github.com/GenericMappingTools/pygmt/blob/master/CONTRIBUTING.md#general-guidelines), you can split it into 2 PRs, 1 for plotting modules (this PR), 1 for data processing modules. Just ask @willschlitzer to review if you think it's ready, I think he'll be keen and it's his ideal timezone now too.

@meghanrjones I changed this PR to just be for plotting functions and I'm about to tackle the non-plotting functions. I looked through your fork's branches and it looks like you haven't started on that.

Co-authored-by: Dongdong Tian <[email protected]>
@seisman
Copy link
Member

seisman commented Feb 14, 2021

Perhaps we can just ignore my last-minute suggestions and merge this PR as it is? @weiji14

@weiji14
Copy link
Member

weiji14 commented Feb 14, 2021

Perhaps we can just ignore my last-minute suggestions and merge this PR as it is? @weiji14

They seem to be fair suggestions, should be ok to just batch commit them before merging.

@seisman
Copy link
Member

seisman commented Feb 14, 2021

I can't do the batch commit, possibly because this PR comes from @meghanrjones's own fork.

@weiji14
Copy link
Member

weiji14 commented Feb 14, 2021

I can't do the batch commit, possibly because this PR comes from @meghanrjones's own fork.

Ah I see. Let's just merge it then (and apply the changes in another small PR?) since it's a public holiday in the US. And we should add her to the PyGMT team so she can create branches on GenericMappingTools/pygmt next time.

@seisman
Copy link
Member

seisman commented Feb 14, 2021

Let's just merge it then.

Great!

apply the changes in another small PR?

I'm heading to the grocery. Feel free to many small changes in PRs.

And we should add her to the PyGMT team so she can create branches on GenericMappingTools/pygmt next time.

She is already in the python team.

@weiji14
Copy link
Member

weiji14 commented Feb 14, 2021

Ah ok, must have missed one of the messages. I'll merge this in in a bit.

@maxrjones
Copy link
Member Author

Should I batch commit them now?

@weiji14
Copy link
Member

weiji14 commented Feb 14, 2021

Oh hi @meghanrjones! Sorry for bothering you on a weekend/public holiday. Yes go ahead please!

@willschlitzer
Copy link
Contributor

Should I batch commit them now?

Please do (assuming you agree with the proposed changes)!

@weiji14
Copy link
Member

weiji14 commented Feb 14, 2021

/format

And also, could you click on the 'Update branch' button (or check the 'Allow updates from maintainers' box so we can update it for you). Thanks.

@maxrjones
Copy link
Member Author

I can create branches from GenericMappingTools in the future if that's preferred. Sorry, I didn't want to make any assumptions when setting up this branch.

pygmt/src/grdimage.py Outdated Show resolved Hide resolved
@weiji14
Copy link
Member

weiji14 commented Feb 14, 2021

I can create branches from GenericMappingTools in the future if that's preferred. Sorry, I didn't want to make any assumptions when setting up this branch.

Yes please! I realize that this probably needs to be in CONTRIBUTING.md (create branches on GenericMappingTools) since Will was doing branches on his own fork for a while too.

And sorry for all the haste, just that we really want to finish that v0.3.0 milestone (but that shouldn't come at the expense of people's lives, not everyone is or should be a workaholic).

@maxrjones
Copy link
Member Author

I can create branches from GenericMappingTools in the future if that's preferred. Sorry, I didn't want to make any assumptions when setting up this branch.

Yes please! I realize that this probably needs to be in CONTRIBUTING.md (create branches on GenericMappingTools) since Will was doing branches on his own fork for a while too.

And sorry for all the haste, just that we really want to finish that v0.3.0 milestone (but that shouldn't come at the expense of people's lives, not everyone is or should be a workaholic).

No problem, I'll be free for the next half hour or so and if this extends beyond that then I'll click the box for you all to be able to finish it up.

@maxrjones
Copy link
Member Author

There was an unescaped return in decorators.py that was causing a space in the perspective arguments. I added a fix to this PR, but feel free to change it back if needed. I won't be able to look at this again today, so feel free to commit new suggestions as needed @weiji14, @willschlitzer, and @seisman.

Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All this attention to detail! Alright, merging this now.

@weiji14 weiji14 merged commit 57da40a into GenericMappingTools:master Feb 14, 2021
@maxrjones maxrjones mentioned this pull request Feb 16, 2021
5 tasks
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
Update docstrings in basemap, coast, colorbar, contour,
grdcontour, grdimage, grdview, image, inset, legend, logo,
plot, plot3d, and text following the convention in GenericMappingTools#631.
Also fixed an unescaped return in decorators.py.

Co-authored-by: Dongdong Tian <[email protected]>
Co-authored-by: Will Schlitzer <[email protected]>
Co-authored-by: Wei Ji <[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.

4 participants