| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
| warnings.warn('The following kwargs were not used by contour: ' + | ||
| s) | ||
|
|
||
| @cbook.deprecated("3.1") |
There was a problem hiding this comment.
Did we decide to deprecate self.ax?
Sorry, something went wrong.
|
I'm -0.5 on this change. The implicit API is a bit goofy anyway, but there are plenty of reasons to have the colorbar steal from the current axes rather than the mappable's axes. I guess I think its more unexpected for plt.subplots(1, 2, 1)
pcm = plt.pcolormesh(X)
plt.subplots(1, 2, 2)
plt.plot(y)
plt.colorbar(pcm) to put the colorbar next to the first subplot than next to the one I specified last. |
Sorry, something went wrong.
There was a problem hiding this comment.
Ok for the warning.
-0.5 for on changing from self.ax to self.axes. This is quite inconsistently used throughout the code base (168 vs. 284 occurrences). If we touch this, I'd favor a separate PR going for all the changes. Changing only some here is just patchwork.
Sorry, something went wrong.
|
For the change to colorbar() to work we need ScalarMappables to have an .axes, so ContourSets need to grow such an attribute (... or we need to special-case ContourSets in the colorbar() code, which seems worse). We don't have to deprecate .ax, we can leave both of them around, but it seems just as good to make things consistent. While .ax and .axes may be nearly as common in raw counts in the codebase, I'd guess .axes is much more common in actual use because that's what Artists have, and Artists also share a lot of code in the base class, whereas .ax is on a couple of custom non-Artist things (e.g. ContourSets) with each their own codepaths. As for plt.subplots(1, 2, 1) pcm = plt.pcolormesh(X) plt.subplots(1, 2, 2) plt.plot(y) plt.colorbar(pcm) if you make your plots vertically (211 and 212 instead of 121 and 122), I think the new behavior is better... TBH I don't really care about plt.colorbar(), but at least for fig.colorbar() I'd rather not invoke the notion of "current axes". |
Sorry, something went wrong.
OK, I agree with that. I don't object to axes being universal versus ax, but I wasn't aware we had decided that. And as noted above an effort should be made to do it everywhere. Understood about needing the attribute. |
Sorry, something went wrong.
|
We haven't decided it, it's just needed for this specific PR to work (and again I can undeprecate .ax for this specific PR if you prefer). Making the change everywhere is absolutely not the point of this PR. If you insist we can also make plt.colorbar() use the current axes but fig.colorbar() the mappable's axes, but that seems worse... |
Sorry, something went wrong.
|
👍, but we should decide on the deprecation... |
Sorry, something went wrong.
Thanks for the explanation. Having ax and axes is not good either. Even though it's not the target of this PR, can we decide on a global change from ax to axes in attribute names? That would allow to keep the deprecation in here and add the remaining ones in another PR. |
Sorry, something went wrong.
|
I'd be happy with the change but I'm sure someone will complain about useless code churn, etc :) |
Sorry, something went wrong.
|
@anntzer To move this forward, you could split out the ax -> axes deprecation and move that to #16058. It's better to discuss it in that context. |
Sorry, something went wrong.
|
OK, I moved the relevant changes to #16058 and will rebase this one on top of it once it goes in. |
Sorry, something went wrong.
|
rebased |
Sorry, something went wrong.
|
Actually, looking at this again, do we want to have the same behavior for pyplot.colorbar too? Obviously pyplot.colorbar gets to know what the current image (gci()) is, but it likely makes sense to always use gci()'s axes, not the current axes? (for the same reason why this makes sense for the OO API) |
Sorry, something went wrong.
|
Added the warning for pyplot.colorbar as a separate commit. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
PR Summary
See discussion at #12333 (comment). Already 👍'd by @tacaswell :)
Edit: also closes #15986 indirectly.
Edit: will be rebased over #16058 once that goes in.
PR Checklist