Wrap rose #794

michaelgrund · 2021-01-17T17:15:58Z

Description of proposed changes

As discussed in #786 this is wrapping the GMT function rose. Would be happy to receive any kind of feedback.

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.

Notes

You can write /format in the first line of a comment to lint the code automatically

As discussed in GenericMappingTools#786 this is wrapping the GMT function **rose**.

Tests for wrapping rose.

michaelgrund · 2021-01-17T17:25:47Z

/format

weiji14

Hi @michaelgrund, this is looking excellent! I've had a quick skim and you're ticking off all the right boxes so far (well documented keyword arguments, top notch unit tests). Will try and give this a proper review once I have time (just busy right now finishing a thesis and preparing for a conference) so I'll ping the other devs @GenericMappingTools/python-maintainers to get some more eyes on this.

Oh, and this is optional, but you can also add a gallery example for rose here in this PR or in a separate Pull Request 😉 I see you're making a few minor changes still, so keep up the good work! Feel free to let us know if you encounter any trouble with anything too.

willschlitzer · 2021-01-18T02:46:04Z

Why isn't @check_figures_equal used in more of the tests? Per the contributing guidelines, that is the recommended method to use for testing plots, if possible. As far as I can tell (I'm still relatively new to this), @check_figures_equal would work in all of the tests that currently use @pytest.mark.mpl_image_compare.

michaelgrund · 2021-01-18T17:42:49Z

Why isn't @check_figures_equal used in more of the tests? Per the contributing guidelines, that is the recommended method to use for testing plots, if possible. As far as I can tell (I'm still relatively new to this), @check_figures_equal would work in all of the tests that currently use @pytest.mark.mpl_image_compare.

I thought providing a mixture would be a good choice @willschlitzer. However, I'm fine with adding a few more tests using @check_figures_equal .

michaelgrund · 2021-01-18T18:11:42Z

/format

seisman · 2021-01-18T20:46:24Z

I thought providing a mixture would be a good choice @willschlitzer. However, I'm fine with adding a few more tests using @check_figures_equal .

We're trying our best to avoid using @pytest.mark.mpl_image_compare, as it stores big image files in the git repository.

pygmt/src/rose.py

maxrjones · 2021-03-31T20:17:41Z

In case you are interested in including an alias for this option (not necessary before merging IMO), GenericMappingTools/gmt#5056 will add documentation to the GMT reST files for a rose option (-N) that was previously only documented in the command line usage info.

Co-authored-by: Dongdong Tian <[email protected]>

michaelgrund · 2021-03-31T20:51:28Z

In case you are interested in including an alias for this option (not necessary before merging IMO), GenericMappingTools/gmt#5056 will add documentation to the GMT reST files for a rose option (-N) that was previously only documented in the command line usage info.

I would do it after merging, potentially together with implementing the inquire option.

pygmt/tests/test_rose.py

seisman · 2021-04-01T01:35:45Z

examples/gallery/embellishments/rose.py

@@ -0,0 +1,44 @@
+"""


According to this comment: #876 (comment), the rose example should be put into the "Histogram" category.

Edit: As the "Historagm" category is not created yet, it's OK to leave the example here now. We can create the category and move the example later.

Ok fine, to keep track will open a new issue regarding that point.

pygmt/tests/test_rose.py

pygmt/src/rose.py

seisman · 2021-04-01T01:48:42Z

The PR looks good. I just made some suggestions. The two comments about refactoring the tests can be addressed in a separate PR.

Co-authored-by: Dongdong Tian <[email protected]>

pygmt/tests/test_rose.py

Co-authored-by: Dongdong Tian <[email protected]>

michaelgrund · 2021-04-01T06:33:11Z

Thanks for the thorough review and all the comments and improvements @weiji14 and @seisman.

weiji14 · 2021-04-01T08:33:03Z

Thanks for the thorough review and all the comments and improvements @weiji14 and @seisman.

No, thank you for your patience and doing all the side tasks like adding a new tutorial dataset! Feel free to Squash and Merge this PR in yourself @michaelgrund, be sure to change the commit message by summarizing what was done (and remove all those lines with "* Update base_plotting.py" or similar). You can also keep just a single instance of the "Co-authored-by ..." for every single person that helped. Let me know if it's unclear.

michaelgrund · 2021-04-01T11:19:52Z

Some tests still fail, are there any obvious reasons for that @seisman @weiji14 ?

seisman · 2021-04-01T14:12:25Z

Something wrong with the GitHub macOS hosts. I think they're unrelated. This PR is good to merge.

seisman · 2021-04-01T14:22:32Z

I just merged the PR using my admin privileges.

* Wrap rose * Add tests * Add a gallery example Co-authored-by: Will Schlitzer <[email protected]> Co-authored-by: Dongdong Tian <[email protected]> Co-authored-by: Wei Ji <[email protected]> Co-authored-by: Meghan Jones <[email protected]>

Wrap rose

728eec4

As discussed in GenericMappingTools#786 this is wrapping the GMT function **rose**.

vercel bot temporarily deployed to Preview January 17, 2021 17:16 Inactive

Create test_rose.py

49044ff

Tests for wrapping rose.

vercel bot temporarily deployed to Preview January 17, 2021 17:22 Inactive

Update index.rst

009dc41

[format-command] fixes

dc999d2

michaelgrund added the feature Brand new feature label Jan 17, 2021

michaelgrund requested a review from weiji14 January 17, 2021 17:27

vercel bot temporarily deployed to Preview January 17, 2021 17:29 Inactive

Update base_plotting.py

048cfa0

vercel bot temporarily deployed to Preview January 17, 2021 18:25 Inactive

Update base_plotting.py

09fb0e1

vercel bot temporarily deployed to Preview January 17, 2021 21:21 Inactive

updated docstrings

f33fd7d

vercel bot temporarily deployed to Preview January 17, 2021 21:38 Inactive

weiji14 reviewed Jan 17, 2021

View reviewed changes

weiji14 requested a review from a team January 17, 2021 21:47

add baseline plots for testing rose

5aa59c6

vercel bot temporarily deployed to Preview January 18, 2021 17:40 Inactive

Update base_plotting.py

8d368db

vercel bot temporarily deployed to Preview January 18, 2021 18:10 Inactive

[format-command] fixes

39c64ec

vercel bot temporarily deployed to Preview January 18, 2021 18:16 Inactive

michaelgrund marked this pull request as draft January 18, 2021 18:47

Merge branch 'master' into patch-2

132757a

seisman reviewed Mar 31, 2021

View reviewed changes

pygmt/src/rose.py Outdated Show resolved Hide resolved

Update pygmt/src/rose.py

b6ff595

Co-authored-by: Dongdong Tian <[email protected]>

vercel bot temporarily deployed to Preview March 31, 2021 20:42 Inactive

seisman reviewed Apr 1, 2021

View reviewed changes

pygmt/tests/test_rose.py Outdated Show resolved Hide resolved

seisman reviewed Apr 1, 2021

View reviewed changes

Apply suggestions from code review

93eb24e

Co-authored-by: Dongdong Tian <[email protected]>

vercel bot temporarily deployed to Preview April 1, 2021 02:03 Inactive

Update pygmt/src/rose.py

950709e

Co-authored-by: Dongdong Tian <[email protected]>

vercel bot temporarily deployed to Preview April 1, 2021 02:10 Inactive

seisman approved these changes Apr 1, 2021

View reviewed changes

Remove whitespace before return fig

7789ee3

Co-authored-by: Dongdong Tian <[email protected]>

vercel bot temporarily deployed to Preview April 1, 2021 02:32 Inactive

Wei Ji and others added 2 commits April 1, 2021 15:45

Merge branch 'master' into patch-2

9c62d4b

Use pytest fixtures for sample data to test rose

da6b829

vercel bot temporarily deployed to Preview April 1, 2021 02:46 Inactive

michaelgrund mentioned this pull request Apr 1, 2021

Create new gallery category "Histogram" #1156

Closed

Merge branch 'master' into patch-2

415fde4

vercel bot temporarily deployed to Preview April 1, 2021 10:23 Inactive

seisman removed the final review call This PR requires final review and approval from a second reviewer label Apr 1, 2021

seisman merged commit e131d26 into GenericMappingTools:master Apr 1, 2021

michaelgrund deleted the patch-2 branch April 20, 2021 06:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrap rose #794

Wrap rose #794

michaelgrund commented Jan 17, 2021 •

edited

Loading

michaelgrund commented Jan 17, 2021

weiji14 left a comment •

edited

Loading

willschlitzer commented Jan 18, 2021

michaelgrund commented Jan 18, 2021

michaelgrund commented Jan 18, 2021

seisman commented Jan 18, 2021

maxrjones commented Mar 31, 2021

michaelgrund commented Mar 31, 2021

seisman Apr 1, 2021 •

edited

Loading

michaelgrund Apr 1, 2021

seisman commented Apr 1, 2021

michaelgrund commented Apr 1, 2021

weiji14 commented Apr 1, 2021

michaelgrund commented Apr 1, 2021

seisman commented Apr 1, 2021

seisman commented Apr 1, 2021

Wrap rose #794

Wrap rose #794

Conversation

michaelgrund commented Jan 17, 2021 • edited Loading

michaelgrund commented Jan 17, 2021

weiji14 left a comment • edited Loading

Choose a reason for hiding this comment

willschlitzer commented Jan 18, 2021

michaelgrund commented Jan 18, 2021

michaelgrund commented Jan 18, 2021

seisman commented Jan 18, 2021

maxrjones commented Mar 31, 2021

michaelgrund commented Mar 31, 2021

seisman Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

michaelgrund Apr 1, 2021

Choose a reason for hiding this comment

seisman commented Apr 1, 2021

michaelgrund commented Apr 1, 2021

weiji14 commented Apr 1, 2021

michaelgrund commented Apr 1, 2021

seisman commented Apr 1, 2021

seisman commented Apr 1, 2021

michaelgrund commented Jan 17, 2021 •

edited

Loading

weiji14 left a comment •

edited

Loading

seisman Apr 1, 2021 •

edited

Loading