Skip to content

turn off the grid after creating colorbar axes #22216

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 14, 2022
Merged

turn off the grid after creating colorbar axes #22216

merged 1 commit into from
Jan 14, 2022

Conversation

pharshalp
Copy link
Contributor

@pharshalp pharshalp commented Jan 12, 2022 •
edited
Loading

PR Summary

This PR turns the grid off for the colorbar axes created automatically (when user doesn't explicitly specify cax) by fig.colorbar or 'plt.colorbar'. This is needed since Auto-removal of grids by pcolor() and pcolormesh is deprecated.

import matplotlib.pyplot as plt
# turn the grid on as a part of the style-sheet
plt.rcParams['axes.grid'] = True  
fig, ax = plt.subplots()
# turn the grid off since "Auto-removal of grids by pcolor() and pcolormesh() is deprecated since 3.5"
ax.grid(False) 
im = ax.pcolormesh([0, 1], [0, 1], [[1]])
# on the main branch (as well as v3.5) this still results in a warning about grid not being turned off
# (since mpl automatically creates cax using the default rcParams and doesn't turn off the grid before
#  plotting the colorbar)
fig.colorbar(im)

possibly closes #21723?

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • [N/A] New features are documented, with examples if plot related.
  • [N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).

@tacaswell tacaswell modified the milestones: v3.5.2, v3.6.0 Jan 12, 2022
@tacaswell
Copy link
Member

I have a slight preference to use a rcParams context manager here over-ride rcParams['axes.grid'] to always be False when creating the Axes in color bar. It is functionally the same, but it communicates to a future reader that we are doing this to (sometimse) over-ride user rcparams because we do not think that they should apply to colorbars (which wile they are Axes objects, they are quite special (among other things one of the spatial axis in them is purely aesthetic!).

@tacaswell
Copy link
Member

Can you also add a test that we do not get warnings on colorbars even with grid-full rcParams settings?

@timhoffm
Copy link
Member

I agree that colorbar axes should not have grids on by default irrespective of the 'axes.grid' setting.

However, technically this is prematurely expiring the deprecation: There can be cases with contours and 'axes.grid'-True styles that have visible grids. It's unlikely that people want that, but if so, we would break their plot.

The clean way is to not warn in the deprecation in case of axes.axisbelow: True (see #21723 (comment)). IMHO we should do this anyway because that also removes the warning for non-colorbar axes in which the grid is below the pcolormesh. In that case the grid was not visible anyway, so we don't need to bother users with a warning.

To be clear: This PR should go in as well, but we should decide whether we want to wait until the deprecation expires or whether it's ok to immediately remove grids from colorbar axes.

@pharshalp
Copy link
Contributor Author

Can you also add a test that we do not get warnings on colorbars even with grid-full rcParams settings?

Sure, I will do that.

@pharshalp
Copy link
Contributor Author

pharshalp commented Jan 13, 2022 •
edited
Loading

I have made an attempt to add a test (not too happy with it though). The test is making sure that there are no warnings raised (instead of checking for absence of the specific warning in question). I would appreciate any suggestions to improve the test.

@tacaswell
Copy link
Member

@pharshalp We talked about this on the call and the consensus is that everyone else but me liked your initial implementation better, can you please switch it back?

Sorry for the thrashing.

We have also have warning -> fail set up in our test running so you do not need that context manager, if it warns, it will fail.

@pharshalp
Copy link
Contributor Author

@tacaswell thanks for the feedback. I have followed the suggestions (reverted to the earlier approach and cleaned up the test).

@QuLogic QuLogic merged commit 719c6b7 into matplotlib:main Jan 14, 2022
@pharshalp pharshalp deleted the cax_grid_off branch January 14, 2022 14:42
@oscargus oscargus mentioned this pull request Aug 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacaswell tacaswell tacaswell approved these changes

@QuLogic QuLogic QuLogic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.

[Bug]: Some styles trigger pcolormesh grid deprecation
4 participants
@pharshalp @tacaswell @timhoffm @QuLogic