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

ENH: add colorbar method to axes #12333

Closed
wants to merge 1 commit into from

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Sep 28, 2018

PR Summary

It has always struck me as strange that if we want to add a colorbar to an axes we have to call if from figure. This change just lets you do ax.colorbar(mappable).

Note that ax.colorbar(mappable, ax=axother) will just ignore the ax argument.

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@jklymak jklymak added this to the v3.1 milestone Sep 28, 2018
%(colorbar_doc)s
"""
self.figure.colorbar(self, mappable, cax=None, ax=self,
use_gridspec=True, **kw)

Choose a reason for hiding this comment

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

I think it needs to be

self.figure.colorbar(mappable, cax=cax, ax=self,
                         use_gridspec=use_gridspec, **kw)

and maybe you want to pop ax out of kw already within this method? Else (I think) it should raise an error if ax is given twice to as argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep! Sorry, should have marked work in progress, I havent' tested it , and it needs tests in test_axes

@ImportanceOfBeingErnest
Copy link
Member

The true advantage of this would be if mappable was optional as well, i.e. if calling ax.colorbar() on an ax which contains a (single) image or collection would - similar to plt.colorbar() - just work out of the box.

@jklymak
Copy link
Member Author

jklymak commented Sep 29, 2018

I think we are trying to avoid implicit APIs like that.

@jklymak
Copy link
Member Author

jklymak commented Sep 29, 2018

... though I guess fig.colorbar implicitly uses gca if ax is not supplied.

@ImportanceOfBeingErnest
Copy link
Member

I would hope fig.colorbar(mappable) uses mappable.axes. Anyways, I would see this similar to ax.legend(). If you don't specify handles and labels it will automatically try to determine the handles and labels to show from the children of ax, via ax.get_legend_handles_labels(). If there are no artists with a label present it will throw a warning. And I think plt.colorbar() does that already. The difference to legend is that if you have more than one ScalarMappable in the axes, ax.colorbar() would need to warn as well.

Thinking that even one step further, fig.legend() also does not require any arguments. So one could also make the mappable optional for fig.colorbar(ax=ax) as well.

"""
ax = kw.pop('ax', None)
if ax is not None:
warnings.warn('Supplied ax argument ignored')
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just make this an error, not much point of being tolerant in a new API...

Choose a reason for hiding this comment

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

I'd say it pretty much depends on how the documentation is written. If it says "allowed kwargs are all matplotlib.figure.Figure.colorbar arguments", then it should warn. If it explicitely says "allowed kwargs are all matplotlib.figure.Figure.colorbar arguments, except ax", it can error out.

Copy link
Member

Choose a reason for hiding this comment

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

👍 to raising. In someplaces we have to tolerate ignoring input because it is old API, but we should not add ignored kwargs in new API.

@WeatherGod
Copy link
Member

WeatherGod commented Sep 29, 2018 via email

@ImportanceOfBeingErnest
Copy link
Member

a colorbar is not owned by an axes, it is owned by the figure.

From a user's perspective, a colorbar most often "belongs" to an axes. Also it strongly interferes with it in the default case where it steals space from the axes. (In which case one could argue inversely as to why a figure level function is allowed to cut axes.)

@jklymak
Copy link
Member Author

jklymak commented Sep 29, 2018

I would hope fig.colorbar(mappable) uses mappable.axes.

if ax is None:
ax = self.gca()

Thats the problem w/ implicit APIs, the API sometimes doesn't do what you expect 😉 Not 100% against making this more implicit, but axes can have more than one mappable, so I find this a bit dangerous.

@WeatherGod I can see that viewpoint. But given that 90%+ of colorbars are attached to a single axes, I think the shortcut proposed here is reasonable API. The user never really sees our organization so they don't really need to know about it. OTOH, the drawback is that it hides the more flexible .Figure.colorbar.

WRT the docs, they are a bit of a mess, so that can/should be cleaned up. But I'm not willing to do it yet because how much needs to be done depends on whether this new API is deemed acceptable. If folks are OK w/ the new API, I can also refactor the docs a bit so plt.colorbar, figure.colorbar, and axes.colorbar have more specific signature descriptions, and then the kwargs are shared.

@tacaswell
Copy link
Member

I am also -0.5 to -0 on this for the same reasons @WeatherGod said. Managing child axes is the business of the figure. I am also not sure that the cax kwarg makes a whole lot of sense an the Axes object

I am nominally 👍 on switching Figure.colorbar to steal from the axes of the mappable object rather than the figure's current axes.

@WeatherGod
Copy link
Member

WeatherGod commented Sep 29, 2018 via email

@jklymak
Copy link
Member Author

jklymak commented Sep 29, 2018

I just think ax.colorbar is more discoverable and what I instinctively think of doing first before I remember I can’t do that. And I think I’m as familiar with how colorbars are implemented as most users. But I don’t feel at all strongly about this if it’s not a widely shared view and just satisfying a personal mental tick.

@ImportanceOfBeingErnest
Copy link
Member

It is a very matplotlib specific implementation detail that a legend is a child artist of an axes, while a colorbar needs its own axes to reside in. From the user's "I want to show an explanation of the contents of my plot"-perspective this distinction is unnecessary and confusing. At some point the user will need to understand the difference (e.g. in the case of clearing the axes mentionned above), but with the proposed ax.colorbar the required level of understanding is lower for the average task of creating a colorbar next to an axes. Or the other way around: a user can do more stuff at the same level of understanding before he needs to acquire new knowledge about further implementation details.

image

Also relevant: Recently a new ax.inset_axes got introduced. This creates an axes, starting from an axes. With the argument that only figure methods should create axes, this would have needed to be fig.inset_axes(ax=ax).

@jklymak
Copy link
Member Author

jklymak commented Sep 30, 2018

Also relevant: Recently a new ax.inset_axes got introduced. This creates an axes, starting from an axes. With the argument that only figure methods should create axes, this would have needed to be fig.inset_axes(ax=ax).

Well, just to clarify, the inset axes is actually a child artist of the axes object because we now allow child axes. Not that I am suggesting we should refactor colorbar so that single-axes colorbars are child axes...

@anntzer
Copy link
Contributor

anntzer commented Sep 30, 2018

My initial reaction was that I like the proposed API, but thinking about it again after reading the other comments I guess the most logical API would be to have ScalarMappable.colorbar(ax=self.axes, *, cax, **kwargs)?

@jklymak
Copy link
Member Author

jklymak commented Oct 3, 2018

Any other opinions on this? So far the consensus seems mildly opposed, with one vote for scalarmappable.colorbar 😉 Happy to scrub it - its maybe more discoverable, but on the other hand it makes it harder to discover the more flexible figure.colorbar

@jklymak
Copy link
Member Author

jklymak commented Oct 5, 2018

Closing due to lack of love for the idea 😉

@jklymak jklymak closed this Oct 5, 2018
@jklymak jklymak deleted the enh-add-colorbar-axes branch October 5, 2018 15:27
@jklymak
Copy link
Member Author

jklymak commented Oct 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants