Wrap rose

[michaelgrund]

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

[michaelgrund]

/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]

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]

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]

/format

[seisman]

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.

[maxrjones]

Really excellent contribution!
I made some suggestions based on my interpretation from the guidelines and pull requests associated with #631 and https://github.com/GenericMappingTools/pygmt/blob/master/CONTRIBUTING.md#example-code-standards that references to parameters should be formatted as code.
I only have one question about the actual implementation (maybe I am missing something here): why include an alias of inquire for -I if it does not work?

Thanks for all the improvements @meghanrjones. As discussed in #896 we plan to add the inquire option later in a separate PR.

My preference would be to not document the alias before the functionality is implemented, because users will not likely be aware of the plan for a non-plotting rose method and may be confused by the NotImplementedError since there isn't a warning about this in the docstring. But, I respect your decisions here if you prefer to put in the documentation for the alias before #896 is addressed.

[michaelgrund]

Really excellent contribution!
I made some suggestions based on my interpretation from the guidelines and pull requests associated with #631 and https://github.com/GenericMappingTools/pygmt/blob/master/CONTRIBUTING.md#example-code-standards that references to parameters should be formatted as code.
I only have one question about the actual implementation (maybe I am missing something here): why include an alias of inquire for -I if it does not work?

Thanks for all the improvements @meghanrjones. As discussed in #896 we plan to add the inquire option later in a separate PR.

My preference would be to not document the alias before the functionality is implemented, because users will not likely be aware of the plan for a non-plotting rose method and may be confused by the NotImplementedError since there isn't a warning about this in the docstring. But, I respect your decisions here if you prefer to put in the documentation for the alias before #896 is addressed.

Good point, what do the others think about this?

[maxrjones]

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.

[michaelgrund]

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.

[seisman]

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.

[michaelgrund]

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

[seisman]

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

[michaelgrund]

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

[weiji14]

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]

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

[seisman]

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

[seisman]

I just merged the PR using my admin privileges.