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 common alias panel (-c) to all plotting functions #853

Merged
merged 5 commits into from
Feb 11, 2021

Conversation

weiji14
Copy link
Member

@weiji14 weiji14 commented Feb 8, 2021

Description of proposed changes

Used to advance to the selected subplot panel. See https://docs.generic-mapping-tools.org/6.1/gmt.html#c-full,
https://github.com/GenericMappingTools/gmt/blob/6.1.1/doc/rst/source/explain_-c_full.rst_, and https://docs.generic-mapping-tools.org/6.1/cookbook/options.html#selecting-subplot-panels-the-c-option. Needed for the subplot wrapper at #822 to resolve issue #20.

Questions:

  1. Just confirming that we want to go with ax (as with matplotlib) rather than panel (used by GMT). See Wrap subplot using with statement #822 (comment). Ok, going with panel
  2. Haven't tested to see if -c works on inset (https://docs.generic-mapping-tools.org/6.1/inset.html), i.e. putting an inset in a subplot panel. If it works, then I'll add the alias to the pygmt/src/inset.py file.

This can be merged before #822 to reduce the diff there.

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 8, 2021
@weiji14 weiji14 added this to the 0.3.0 milestone Feb 8, 2021
@seisman
Copy link
Member

seisman commented Feb 8, 2021

  • Just confirming that we want to go with ax (as with matplotlib) rather than panel (used by GMT). See #822 (comment).

I'm OK with both, although I prefer panel. When I learn matplotlib, I find it difficult to understand the differences between "axes" and "axis" (likely because I'm not a native English speaker). BTW, it seems Julia Plot.jl uses subplot (http://docs.juliaplots.org/latest/layouts/),

I think it doesn't work, to call inset, you need to have a larger map first.

@seisman
Copy link
Member

seisman commented Feb 8, 2021

I also find this (https://matplotlib.org/tutorials/intermediate/artists.html):

Most of you are probably familiar with the Subplot, which is just a special case of an Axes that lives on a regular rows by columns grid of Subplot instances.

If I understand it correctly, GMT's subplots also live on a regular rows by columns grid, so it should be called subplot, not axes, right?

@weiji14
Copy link
Member Author

weiji14 commented Feb 8, 2021

  • Just confirming that we want to go with ax (as with matplotlib) rather than panel (used by GMT). See #822 (comment).

I'm OK with both, although I prefer panel. When I learn matplotlib, I find it difficult to understand the differences between "axes" and "axis" (likely because I'm not a native English speaker).

Oh, and just FYI, there's a Python library called Panel for making dashboards (i.e. interactive subplots). To be fair, I don't really get the "axes" and "axis" terminology used by matplotlib either, "axes" should be the plural of "axis" but they don't use it that way.

I think it doesn't work, to call inset, you need to have a larger map first.

Ok, will leave out inset for now then.

I also find this (https://matplotlib.org/tutorials/intermediate/artists.html):

Most of you are probably familiar with the Subplot, which is just a special case of an Axes that lives on a regular rows by columns grid of Subplot instances.

If I understand it correctly, GMT's subplots also live on a regular rows by columns grid, so it should be called subplot, not axes, right?

Not sure if it's a good idea to use 'subplot' as an alias for 'c' (if that's what you mean). We'll hit into the same problem as with def text(text=...) as in #808 (comment).

With all that said, I would still argue that using ax is the way to go, considering that both pandas.plot and xarray.plot has a kwarg called ax. It might be useful to use ax too (or perhaps confusing) to integrate with other Python libraries (c.f. #374, #366 (comment)) in the future.

@seisman
Copy link
Member

seisman commented Feb 8, 2021

Not sure if it's a good idea to use 'subplot' as an alias for 'c' (if that's what you mean). We'll hit into the same problem as with def text(text=...) as in #808 (comment).

Yes, I agree, subplot may be a bad idea.

With all that said, I would still argue that using ax is the way to go, considering that both pandas.plot and xarray.plot has a kwarg called ax. It might be useful to use ax too (or perhaps confusing) to integrate with other Python libraries (c.f. #374, #366 (comment)) in the future.

pandas.plot and xarray.plot have the ax kwarg because they are integrating well with matplotlib. However, pygmt can't insert a GMT figure to a matplot axes, right?

@willschlitzer
Copy link
Contributor

Little late to the party, but my vote would be panel over ax. I understand the potential conflicts of panel and the familiarity of ax with other plotting packages, but I think panel makes more sense in this context, as the specific panel to be plotting on is being set, not just the axis/axes (even though they refer to the same thing).

@weiji14
Copy link
Member Author

weiji14 commented Feb 9, 2021

Little late to the party, but my vote would be panel over ax. I understand the potential conflicts of panel and the familiarity of ax with other plotting packages, but I think panel makes more sense in this context, as the specific panel to be plotting on is being set, not just the axis/axes (even though they refer to the same thing).

Ok, if we want to go with panel, then we should change fig.sca too (short for set current ax) in #822 to something else. fig.set_panel perhaps?

@willschlitzer
Copy link
Contributor

Little late to the party, but my vote would be panel over ax. I understand the potential conflicts of panel and the familiarity of ax with other plotting packages, but I think panel makes more sense in this context, as the specific panel to be plotting on is being set, not just the axis/axes (even though they refer to the same thing).

Ok, if we want to go with panel, then we should change fig.sca too (short for set current ax) in #822 to something else. fig.set_panel perhaps?

I think set_panel works well. I particularly like that it doesn't use an acronym.

@seisman
Copy link
Member

seisman commented Feb 9, 2021

Little late to the party, but my vote would be panel over ax. I understand the potential conflicts of panel and the familiarity of ax with other plotting packages, but I think panel makes more sense in this context, as the specific panel to be plotting on is being set, not just the axis/axes (even though they refer to the same thing).

Ok, if we want to go with panel, then we should change fig.sca too (short for set current ax) in #822 to something else. fig.set_panel perhaps?

fig.set_panel looks good to me.

@weiji14 weiji14 changed the title Add common alias ax (-c) to all plotting functions Add common alias panel (-c) to all plotting functions Feb 10, 2021
Copy link
Member Author

@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.

Ok I've renamed ax -> panel. Will do the fig.set_panel rename later in #822, but just one comment/question.

pygmt/helpers/decorators.py Outdated Show resolved Hide resolved
pygmt/helpers/decorators.py Outdated Show resolved Hide resolved
pygmt/helpers/decorators.py Outdated Show resolved Hide resolved
Co-Authored-By: Dongdong Tian <seisman.info@gmail.com>
@weiji14 weiji14 merged commit 1e25fd4 into master Feb 11, 2021
@weiji14 weiji14 deleted the common-alias-c-subplot branch February 11, 2021 22:04
@weiji14 weiji14 mentioned this pull request Mar 20, 2021
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
…Tools#853)

Used to select a specific subplot panel.
See https://docs.generic-mapping-tools.org/6.1/gmt.html#c-full,
https://github.com/GenericMappingTools/gmt/blob/6.1.1/doc/rst/source/explain_-c_full.rst_,
and https://docs.generic-mapping-tools.org/6.1/cookbook/options.html#selecting-subplot-panels-the-c-option.
Needed for the `subplot` wrapper at GenericMappingTools#822.

* Add panel (c) to list of necessary arguments in basemap
* Rename alias ax to panel to follow upstream GMT
* Update description of common alias c to be more Pythonic

Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
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.

3 participants