-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Deprecate auto-removal of grid by pcolor/pcolormesh. #15604
Conversation
There was a problem hiding this 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).
@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. |
There are some styles that define Can we agree (which I find reasonable and is the current default), that |
What is the rationale for that default? Here is a |
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 Concerning this PR, the grid lines would still be there after, but they will lie beneath the |
Well, if they warrant different defaults they should warrant different rcParams, I guess... |
But it's a style. There's just one global |
I'm sorry if I'm being dense, but I'm not following you. |
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)
because of the style topic (alleviated by the fact that all provided styles use To sum up, and look towards a resolution:
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. |
I wrote issue #15600, just to add some details. |
ce1daf7
to
968b96e
Compare
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. |
rebased |
968b96e
to
ac0448b
Compare
ooops, something changed! |
ac0448b
to
c5ffaae
Compare
hopefully fixed... |
Close? |
trying again... |
c5ffaae
to
30b6df9
Compare
Still some deprecation warnings. |
As it turns out, avoiding spurious warnings is a bit trickier than I expected: if 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 |
🤷♂️ probably ok with no deprecation, but I'd like to have somebody else agree. |
That makes sense to me, but maybe we need yet a third opinion. |
I'm also OK with dropping the deprecation. |
Actually I was confused when I wrote
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. |
PR Summary
Closes #15600.
PR Checklist