Skip to content

turn off the grid after creating colorbar axes#22216

Merged
QuLogic merged 1 commit into
matplotlib:mainfrom
pharshalp:cax_grid_off
Jan 14, 2022
Merged

turn off the grid after creating colorbar axes#22216
QuLogic merged 1 commit into
matplotlib:mainfrom
pharshalp:cax_grid_off

Conversation

@pharshalp

@pharshalp pharshalp commented Jan 12, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

tacaswell commented Jan 12, 2022

Copy link
Copy Markdown
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

tacaswell commented Jan 12, 2022

Copy link
Copy Markdown
Member

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

@timhoffm

timhoffm commented Jan 12, 2022

Copy link
Copy Markdown
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

pharshalp commented Jan 13, 2022

Copy link
Copy Markdown
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

pharshalp commented Jan 13, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

tacaswell commented Jan 13, 2022

Copy link
Copy Markdown
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

pharshalp commented Jan 13, 2022

Copy link
Copy Markdown
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