Skip to content

Deprecate auto-removal of grid by pcolor/pcolormesh. #15604

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

Merged
merged 1 commit into from
May 20, 2021

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Nov 5, 2019

PR Summary

Closes #15600.

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

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

I'm blocking, because I believe deactivating the grid is actually the right thing to do to achive reasonable default behavior for plot styles with grids enabled by default (see #15600 (comment)).

This needs thorough consideration including implications for plot styles with grids enabled by default. Will lift if there is a major consensus that auto-removal is overall bad (removing a possibly explicitly activated grid on a pcolormesh plot is worse than having gridlines in a default ggplot-style pcolormesh plot).

@jklymak
Copy link
Member

jklymak commented Nov 6, 2019

@TimHoffman what do you mean by “grids enabled by default”. Do you mean artists with a grid-like structure? I think such grids are only distinguishable if you have pretty small data sets. I think for many pcolor and image data sets the pixels are too small to line up and a grid line can be helpful.

@timhoffm
Copy link
Member

timhoffm commented Nov 6, 2019

There are some styles that define axes.grid: True (e.g. seaborn and ggplot). This is primarily intended for plot or scatter plots. But it's a global style setting for all Axes. In particular, this is enforced at creation time of the Axes, when we do not know yet what will be plotted into the axes.

Can we agree (which I find reasonable and is the current default), that pcolormesh plots should not be crossed by grid lines by default also in ggplot and similar? If so, the only way to prevent that is swiching off the grid inside pcolormesh.

@jklymak
Copy link
Member

jklymak commented Nov 6, 2019

Can we agree (which I find reasonable and is the current default), that pcolormesh plots should not be crossed by grid lines by default also in ggplot and similar?

What is the rationale for that default? Here is a pcolormesh from today, and while I don't typically have grids on, I don't see why we would think a user who wants them on by default would want them turned off for pcolors?

BottomStress

BottomStressGrid

@timhoffm
Copy link
Member

timhoffm commented Nov 6, 2019

Scatter/line plots and false-color plots are fundamentally different and may warrant different defaults defaults.

Acutally, I just realized that these styles (ggplot, seaborn) use the rcParam axes.axisbelow. That effectively hides the grid behind the QuadMesh.

Concerning this PR, the grid lines would still be there after, but they will lie beneath the QuadMesh. So with this PR, there is no visual change for defaults, and I'm withdrawing my block.

@timhoffm timhoffm dismissed their stale review November 6, 2019 21:53

Dismissed. See above.

@jklymak
Copy link
Member

jklymak commented Nov 6, 2019

Scatter/line plots and false-color plots are fundamentally different and may warrant different defaults defaults.

Well, if they warrant different defaults they should warrant different rcParams, I guess...

@timhoffm
Copy link
Member

timhoffm commented Nov 6, 2019

Well, if they warrant different defaults they should warrant different rcParams, I guess...

But it's a style. There's just one global axes.grid param for all types of plots.

@jklymak
Copy link
Member

jklymak commented Nov 6, 2019

I'm sorry if I'm being dense, but I'm not following you. ax.pcolormesh ignores rcParam['axes.grid'] and just turns the grid off regardless of the style. If you think styles should be able to set a different behaviour for pcolors/images, then doesn't that need something like this PR and a different rcParam?

@timhoffm
Copy link
Member

timhoffm commented Nov 6, 2019

This is getting a bit lengthy, and I doubt it's worth the effort of the discussion.

Technically, you are right, that would be a more general solution. However, historically/empirically it was chosen to just deactivate the grid. My whole point was/is that there's more to it than #15600 (comment)

It's probably actually not too hard to change that (and I think removing that line makes sense): one can check before the call to grid(False) whether the grid is on (which would only happen if explicitly set earlier in the code and then the user relying on pcolor(mesh)() unsetting it...)

because of the style topic (alleviated by the fact that all provided styles use axes.grid=True and axes.axisbelow=True). However, it may potentially affect other styles out there. You could introduce additional rcParams, but these styles would still have to adapt to our code change.

To sum up, and look towards a resolution:

  • almost nobody had a problem with pcolormesh deactivating the grid (first report in 14 years). And it can easily be restored by activating the grid after pcolormesh.
  • almost nobody will see a change if we remove the grid code (hopefully, unless some downstream library uses a certain style settings).

Therefore, I don't care. I've raised my (now only) slight concern. I would just leave this as it was, but if others want to merge I'm fine with that as well.

@AntSimi
Copy link

AntSimi commented Nov 7, 2019

  • almost nobody had a problem with pcolormesh deactivating the grid (first report in 14 years). And it can easily be restored by activating the grid after pcolormesh.

I wrote issue #15600, just to add some details.
In fact i have seen this behaviour since few years, in my real use case (A GUI to explore netCDF data, grid reprensent some meridian and parallel that i want to display, because my plot could be a composite of pcolormesh/scatter/quiver/...) pcolormesh could be call after or before grid, it's depends of user action in the GUI. I added a dirty fix in my real use case: i get before pcolormesh grid state and i apply old state after pcolormesh.
And when i read again my code recently, i decide to create issue, i didn't know it was an historic choice.

@anntzer anntzer force-pushed the deprecate-pcolor-removing-grid branch 4 times, most recently from ce1daf7 to 968b96e Compare January 14, 2020 00:36
@jklymak
Copy link
Member

jklymak commented Apr 23, 2021

I still think this should go in. Arbitrarily removing the grid based on artist type is unexpected and means the user has to do weird work arounds. Of course the version will have to be updated.

@anntzer
Copy link
Contributor Author

anntzer commented Apr 23, 2021

rebased

@anntzer anntzer force-pushed the deprecate-pcolor-removing-grid branch from 968b96e to ac0448b Compare April 23, 2021 16:31
@jklymak
Copy link
Member

jklymak commented Apr 23, 2021

ooops, something changed!

@anntzer anntzer force-pushed the deprecate-pcolor-removing-grid branch from ac0448b to c5ffaae Compare April 23, 2021 16:52
@anntzer
Copy link
Contributor Author

anntzer commented Apr 23, 2021

hopefully fixed...

@jklymak
Copy link
Member

jklymak commented Apr 23, 2021

Close? The warn_deprecated function was deprecated in Matplotlib 3.4 and will be removed two minor releases later.

@anntzer
Copy link
Contributor Author

anntzer commented Apr 23, 2021

trying again...

@anntzer anntzer force-pushed the deprecate-pcolor-removing-grid branch from c5ffaae to 30b6df9 Compare April 23, 2021 17:49
@timhoffm
Copy link
Member

Still some deprecation warnings.

@anntzer
Copy link
Contributor Author

anntzer commented Apr 24, 2021

As it turns out, avoiding spurious warnings is a bit trickier than I expected: if axes.grid: True, or for polar axes (for which polaraxes.grid already default to True), assuming that the user just plots a pcolormesh, then we emit a warning (because pcolormesh will hide the grid), even though by default the grid was not going to be visible anyways unless the user changes the axes limits to go beyond those of the pcolormesh (which was the original issue at hand), as the pcolormesh will be drawn above the grid anyways (by default). Trying to keep a flag to later emit a warning if the grid would have been visible because the axes limits were changed to not match the limits of the pcolormesh seems way more work than worth it.

In other words, if you were not plotting anything other than the pcolormesh, then directly changing pcolormesh to not remove the grid shouldn't affect you (but this PR would emit a spurious warning). In the other cases (e.g. the original #15600), then the new behavior seems to be clearly what you want anyways. Hence, the practical solution may just be directly remove the grid(False) call with no deprecation period?

@timhoffm
Copy link
Member

🤷‍♂️ probably ok with no deprecation, but I'd like to have somebody else agree.

@jklymak
Copy link
Member

jklymak commented Apr 24, 2021

That makes sense to me, but maybe we need yet a third opinion.

@QuLogic QuLogic added the status: needs comment/discussion needs consensus on next step label Apr 26, 2021
@efiring
Copy link
Member

efiring commented May 20, 2021

I'm also OK with dropping the deprecation.

@jklymak jklymak requested a review from timhoffm May 20, 2021 18:58
@jklymak jklymak merged commit d37551a into matplotlib:master May 20, 2021
@anntzer anntzer deleted the deprecate-pcolor-removing-grid branch May 20, 2021 23:53
@anntzer
Copy link
Contributor Author

anntzer commented May 30, 2021

Actually I was confused when I wrote

as the pcolormesh will be drawn above the grid anyways (by default).

as the pcolormesh is normally drawn below the grid and therefore doesn't hide it. Hence I believe the deprecation warning implemented by this PR is basically OK.

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

Successfully merging this pull request may close these issues.

Grid disappear after pcolormesh apply
7 participants