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

Add tutorial for contour maps #705

Merged
merged 29 commits into from
Dec 1, 2020

Conversation

willschlitzer
Copy link
Contributor

Created a tutorial on the usage of the grdcontour method to demonstrate creating a plot with contour lines and adjusting the intervals, annotations, and limits. Per the feedback from @seisman, I'm resubmitting this under it's own branch from my fork and not including the .ipynb file.

doc/index.rst Outdated Show resolved Hide resolved
examples/tutorials/contour-map.py Outdated Show resolved Hide resolved
examples/tutorials/contour-map.py Outdated Show resolved Hide resolved
Remove duplicate word

Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
willschlitzer and others added 2 commits November 25, 2020 20:18
Update order on tutorials

Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
Update default projection
@seisman seisman added the documentation Improvements or additions to documentation label Nov 25, 2020
@seisman seisman added this to the 0.3.0 milestone Nov 25, 2020
Fix typo; "blow" to "below"
@willschlitzer
Copy link
Contributor Author

@seisman Pardon my unfamiliarity with the open source contributing process, but I just want to confirm that you are not waiting on any changes/additional contributions for this PR? Thanks!

@seisman
Copy link
Member

seisman commented Nov 27, 2020

@willschlitzer Sorry for the absence. I'm a little busy yesterday and will leave more reviews/comments in the next few days.

examples/tutorials/contour-map.py Outdated Show resolved Hide resolved
examples/tutorials/contour-map.py Outdated Show resolved Hide resolved
examples/tutorials/contour-map.py Outdated Show resolved Hide resolved
Removed region from the method description because it is not a required arguement

Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
willschlitzer and others added 2 commits November 28, 2020 09:39
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
examples/tutorials/contour-map.py Outdated Show resolved Hide resolved
examples/tutorials/contour-map.py Show resolved Hide resolved
examples/tutorials/contour-map.py Outdated Show resolved Hide resolved
examples/tutorials/contour-map.py Outdated Show resolved Hide resolved
examples/tutorials/contour-map.py Outdated Show resolved Hide resolved
examples/tutorials/contour-map.py Outdated Show resolved Hide resolved
examples/tutorials/contour-map.py Outdated Show resolved Hide resolved
examples/tutorials/contour-map.py Outdated Show resolved Hide resolved
@seisman
Copy link
Member

seisman commented Nov 28, 2020

@willschlitzer One more thing, although it's not mandatory, it would be better if you can wrap the lines to no longer than 88 characters, to make it more readable.

Move grid variable assignment to top of script

Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
willschlitzer and others added 2 commits November 29, 2020 08:08
Adding comment for grid variable

Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
typo correction

Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
@willschlitzer
Copy link
Contributor Author

@seisman Now that I've received feedback on how to document the example code, I decided to commit the example I had to include a colormap on the contour map. It seemed easier to commit it now to include it in this pull request, but please let me know if I overstepped and should have instead waited for a separate pull request.

@seisman
Copy link
Member

seisman commented Nov 29, 2020

@seisman Now that I've received feedback on how to document the example code, I decided to commit the example I had to include a colormap on the contour map. It seemed easier to commit it now to include it in this pull request, but please let me know if I overstepped and should have instead waited for a separate pull request.

It looks good to add a color map.

examples/tutorials/contour-map.py Outdated Show resolved Hide resolved
examples/tutorials/contour-map.py Outdated Show resolved Hide resolved
examples/tutorials/contour-map.py Outdated Show resolved Hide resolved
examples/tutorials/contour-map.py Outdated Show resolved Hide resolved
@seisman
Copy link
Member

seisman commented Nov 29, 2020

@willschlitzer I just leave more comments on your new example. I think the tutorial is pretty good now.

The only issue is that the tutorial is using the "15s" earth relief data. As you may know or not, these earth relief data are stored in the GMT data server, and GMT (or PyGMT) will download the grid tiles for the first time the data is used. Usually, each tile is smaller than 10 Mb, so it's not a problem for users. However, the Linux and Windows agents of GitHub Actions (the machine/service we're using to run tests and build documentation) sometimes can't download data from the GMT data server (#434 (comment)), so we have to cache the data using a macOS agent and let Linux/Windows re-use the caches (#530).

Currently, only the following earth relief data are cached:

gmt which -Ga @earth_relief_10m_p @earth_relief_10m_g \
@earth_relief_30m_p @earth_relief_30m_g \
@earth_relief_01d_p @earth_relief_01d_g \
@earth_relief_05m_g

It would be good if you can modify your examples using a low-resolution grid (05m or 10m, you can increase the region setting), otherwise, we have to cache the 15s earth relief data, too.

Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
willschlitzer and others added 3 commits November 29, 2020 16:51
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
@willschlitzer
Copy link
Contributor Author

@seisman I updated the region setting to "05m". I'm pretty unfamiliar with using the GMT Earth relief data date, but I was unable to use "10m," as I received a NotImplementedError that said it isn't supported for that resolution.

Thanks for all of your feedback (and patience with my mistakes)!

@seisman
Copy link
Member

seisman commented Nov 29, 2020

Please also click the "Update branch" button to merge the latest changes in the master branch to your branch.

image

Copy link
Member

@seisman seisman left a comment

Choose a reason for hiding this comment

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

Now it looks pretty good to me.

@seisman
Copy link
Member

seisman commented Nov 30, 2020

I'll merge this PR tomorrow. @GenericMappingTools/python if you find anything that should be fixed, please leave your comments in the next 24 hours.

@willschlitzer
Copy link
Contributor Author

@seisman Thanks for all of the feedback throughout this process. I look forward to using my lessons learned on future pull requests on this project.

@seisman seisman changed the title Contour map example Add tutorial for contour maps Dec 1, 2020
@seisman seisman merged commit 47516a4 into GenericMappingTools:master Dec 1, 2020
@seisman
Copy link
Member

seisman commented Dec 1, 2020

@willschlitzer As mentioned in #707 (comment), feel free to open a new pull request to add yourself to the AUTHORS.md file.

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