Fix colorbar exponents #22313

jklymak · 2022-01-25T12:11:11Z

PR Summary

Fix offsetText for colorbars with extends so they are above the extends... Closes #22312

Before

After

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

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

jklymak · 2022-01-25T12:14:23Z

... note that horizontal colorbars don't suffer from this problem so nothing was changed for XAxis....

greglucas · 2022-01-25T15:05:26Z

lib/matplotlib/axis.py

+            # Special case for colorbars:
+            bbox = self.axes.spines['outline'].get_window_extent()
+        else:
+            bbox = self.axes.bbox


Would it make sense to update the axes.bbox in Colorbar to be the full spines outline? Move it up a level, so everything is referencing the same full bbox rather than just the text here. I'm not sure if that would mess up the tickers to draw in the extends region or not then...

ax.transAxes comes from the ax.bbox, so I don't think that would work as the axes extends are drawn in transAxes. I appreciate what you are saying though, it is a bit of a hack to special-case colorbars here...

Another alternative is to turn the axis off, use ax.get_tightbbox() and then turn the axis back on again. But I think get_tightbbox can get pretty expensive depending on artists. This is quite fast...

I try to paraphrase: the colorbar axes has additional decorations which are outside the bbox but need to be taken into accout for shifting the offset text? In that case, do we need a concept for "bbox with decorations"?

We do, it's called get_tightbbox. However that is defined on the axes and takes into account the axis, and this exponent is part of the axis, so you get an infinite recursion. I guess we could change get_tightbbox to accept a list of artists to exclude, but I'm not convinced that is any better than this solution. And again get_tightbbox can be very expensive.

Thanks for the explanation. I don't claim to I know how the correct or practical solution. I was just trying to understand what's going on.

jklymak · 2022-01-26T20:44:44Z

... moved to draft. cb.ax.set_title has the sae problem (though that usage is more rare, I expect)

tacaswell · 2022-02-11T02:52:34Z

Power-cycled now that we have fixed CI

QuLogic

Doc failure appears relevant.

lib/matplotlib/axes/_base.py

jklymak · 2022-02-11T07:15:03Z

Yes it does look relevant. Are you trying to get 3.5.2 out the door? I'm away from my computer until next wed (16 feb). Looks like the check for outline in the colorbar spine dict is not correct.

QuLogic · 2022-02-11T22:23:54Z

No, not yet, probably next week.

jklymak · 2022-02-12T08:49:52Z

I can fix the 17th or so, but not before. If anyone wants to push a check to the PR it's not a problem. Thanks!

jklymak · 2022-02-21T08:48:25Z

tutorials/introductory/quick_start.py

@@ -505,7 +505,7 @@ def my_plotter(ax, data1, data2, param_dict):

 pc = axs[1, 1].scatter(data1, data2, c=data3, cmap='RdBu_r')
 fig.colorbar(pc, ax=axs[1, 1], extend='both')
-axs[1, 1].set_title('scatter()');
+axs[1, 1].set_title('scatter()')


these semicolons dont' work anyways, so removed it here...

jklymak · 2022-02-21T12:26:59Z

OK, I admit to being quite stymied by the doc failure. What is happening is that the quick_start code is somehow creating a second figure in the pyplot registry, and this second figure is not quite setup properly. You can check this by adding print statements to the matplotlib_scraper in sphinx_gallery and seeing that it tries to create two figures for this part of the example.

The root of the problem seems to be the deepcopy in norm:

matplotlib/lib/matplotlib/colorbar.py

Line 1161 in b80bc91

norm = copy.deepcopy(self.norm)

If I remove this, then the sphinx scraper doesn't create the extra figure.

The deepcopy arose because of the extra state that @greglucas added to sync colorbars and mappables. The failure is on norms made with _make_norm_from_scale which @anntzer added. If I manually define Norm to have a __deepcopy__ that simply returns the copy, then the failure goes away. I'm hoping either @greglucas or @anntzer can take a look and help us figure out what the heck is going on.

Looking at the code here, it definitely somehow triggers this weirdness, but I'm not understanding why it should.

The reproducer here is to put print statements in sphinx_gallery/scrapers.py in the matplotlib scraper, and see that print(plt.get_fignums()) returns two figures for "Color mapped data" in quick_start.py:

matplotlib/tutorials/introductory/quick_start.py

Line 482 in b80bc91

##############################################################################

when running make html

greglucas · 2022-02-21T16:33:00Z

Strange indeed... This looks like it is also not working correctly on the current docs build on main, although not failing.
https://67051-1385122-gh.circle-artifacts.com/0/doc/build/html/tutorials/introductory/quick_start.html?highlight=quick%20start#color-mapped-data

So, I don't think this PR is a culprit, just hitting a louder failure. I'll try and dig deeper this evening if @anntzer doesn't figure it out before that.

jklymak · 2022-02-21T16:38:26Z

Ah, good catch. Yes, this PR is probably squeaking because the spine is squished to nothing in the weird second figure.

Again, it has something to do with the deepcopy in colorbar. If I remove that it seems to work fine. But the deepcopy has been around for a while, so I suspect some of the recent changes to the norms.

anntzer · 2022-02-21T21:15:41Z

I probably won't have the time to investigate this for a few days, sorry. I'll try to get back to it later...

greglucas · 2022-02-21T22:20:40Z

A fix to remove the deepcopy is in #22523, which solves the docs build here, but does not solve the underlying problem that could crop up somewhere else.

The root cause bisects to: #21597
So, definitely something going on with the deepcopy on transforms. Removing getstate/setstate also makes the example "work" again, but of course causes other issues with pickling. So something with expanding the weakrefs of the parents attribute I think. We could revert that deepcopy removal commit and lose the ability to copy figures as possibly the easiest solution if we hit other issues here.

lib/matplotlib/axis.py

greglucas · 2022-03-04T14:48:22Z

#22523 will fix the doc build for the main branch with this PR, and this shouldn't be an issue on 3.5 as the failing code was introduced after the 3.5.x branched off. I've re-opened a separate issue to track the underlying issue here which is the picklability of norms: #20755

timhoffm · 2022-03-08T02:57:14Z

#22523 is in. @jklymak please rebase to see that the doc build is fixed with that.

FIX: title as well

lumberbot-app · 2022-03-09T00:14:27Z

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

git checkout v3.5.x
git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -m1 698397f95b61d007137ead5e733f6a1b1ecff032

You will likely have some merge/cherry-pick conflict here, fix them and commit:

git commit -am 'Backport PR #22313: Fix colorbar exponents'

Push to a named branch:

git push YOURFORK v3.5.x:auto-backport-of-pr-22313-on-v3.5.x

Create a PR against branch v3.5.x, I would have named this PR:

"Backport PR #22313 on branch v3.5.x (Fix colorbar exponents)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

anntzer · 2022-04-08T20:52:26Z

@jklymak @greglucas Sorry I didn't have the time to look at this; I'm trying to catch up a bit now. If I understand correctly there may still be a bug wrt deepcopying norms made with make_norm_from_scale? (which #22523 works around, but may otherwise may still be present?) Do colorbars need to be involved at all? Do you have a repro I can look at? (other than reverting #22523, as that PR seems like a good idea to me regardless)

greglucas · 2022-04-08T21:01:26Z

Yes, they are still broken and you actually had a patch :) You can add this to the current test:
#20755 (comment)

I also was curious on whether this would be one of the cases where a metaclass might help as being a kind of class factory here, but I didn't think about that too much.

…3.5.x Backport PR #22313 on branch v3.5.x (Fix colorbar exponents)

jklymak added the topic: color/colorbar label Jan 25, 2022

jklymak added this to the v3.5.2 milestone Jan 25, 2022

jklymak added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jan 25, 2022

jklymak force-pushed the fix-colorbar-exponents branch from a97402c to 5d38073 Compare January 25, 2022 12:13

greglucas reviewed Jan 25, 2022

View reviewed changes

jklymak marked this pull request as draft January 26, 2022 20:44

jklymak force-pushed the fix-colorbar-exponents branch from 07bf9b2 to 2fd7a26 Compare February 2, 2022 13:31

jklymak closed this Feb 3, 2022

jklymak reopened this Feb 3, 2022

jklymak marked this pull request as ready for review February 3, 2022 08:08

jklymak force-pushed the fix-colorbar-exponents branch 2 times, most recently from 1933d58 to 4c371d0 Compare February 4, 2022 12:31

tacaswell closed this Feb 11, 2022

tacaswell reopened this Feb 11, 2022

tacaswell approved these changes Feb 11, 2022

View reviewed changes

QuLogic reviewed Feb 11, 2022

View reviewed changes

lib/matplotlib/axes/_base.py Outdated Show resolved Hide resolved

jklymak commented Feb 21, 2022

View reviewed changes

greglucas mentioned this pull request Feb 21, 2022

MNT: Use a context manager to change the norm in colorbar code #22523

Merged

5 tasks

QuLogic reviewed Mar 3, 2022

View reviewed changes

lib/matplotlib/axis.py Show resolved Hide resolved

greglucas approved these changes Mar 4, 2022

View reviewed changes

jklymak added 2 commits March 7, 2022 22:42

FIX: fix offset text for vertical colorbars

6e493b0

FIX: title as well

FIX: ax

707d8a2

jklymak force-pushed the fix-colorbar-exponents branch from 522bc6d to 707d8a2 Compare March 8, 2022 06:44

QuLogic merged commit 698397f into matplotlib:main Mar 9, 2022

lumberbot-app bot added the Still Needs Manual Backport label Mar 9, 2022

anntzer mentioned this pull request Apr 9, 2022

Fix pickling of globally available, dynamically generated norm classes. #22815

Merged

6 tasks

QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Apr 13, 2022

Backport PR matplotlib#22313: Fix colorbar exponents

f6c755c

QuLogic mentioned this pull request Apr 13, 2022

Backport PR #22313 on branch v3.5.x (Fix colorbar exponents) #22844

Merged

2 tasks

QuLogic removed the Still Needs Manual Backport label Apr 13, 2022

jklymak added a commit that referenced this pull request Apr 21, 2022

Merge pull request #22844 from QuLogic/auto-backport-of-pr-22313-on-v…

d8629e9

…3.5.x Backport PR #22313 on branch v3.5.x (Fix colorbar exponents)

jklymak mentioned this pull request May 12, 2022

[Bug]: colorbar axes title clobbers extend arrow #23041

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix colorbar exponents #22313

Fix colorbar exponents #22313

jklymak commented Jan 25, 2022 •

edited

Loading

jklymak commented Jan 25, 2022

greglucas Jan 25, 2022

jklymak Jan 25, 2022

timhoffm Jan 25, 2022

jklymak Jan 25, 2022

timhoffm Jan 26, 2022 •

edited

Loading

jklymak commented Jan 26, 2022

tacaswell commented Feb 11, 2022

QuLogic left a comment

jklymak commented Feb 11, 2022

QuLogic commented Feb 11, 2022

jklymak commented Feb 12, 2022

jklymak Feb 21, 2022

jklymak commented Feb 21, 2022 •

edited

Loading

greglucas commented Feb 21, 2022

jklymak commented Feb 21, 2022

anntzer commented Feb 21, 2022

greglucas commented Feb 21, 2022

greglucas commented Mar 4, 2022

timhoffm commented Mar 8, 2022

lumberbot-app bot commented Mar 9, 2022

anntzer commented Apr 8, 2022

greglucas commented Apr 8, 2022

Fix colorbar exponents #22313

Fix colorbar exponents #22313

Conversation

jklymak commented Jan 25, 2022 • edited Loading

PR Summary

Before

After

PR Checklist

jklymak commented Jan 25, 2022

greglucas Jan 25, 2022

Choose a reason for hiding this comment

jklymak Jan 25, 2022

Choose a reason for hiding this comment

timhoffm Jan 25, 2022

Choose a reason for hiding this comment

jklymak Jan 25, 2022

Choose a reason for hiding this comment

timhoffm Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

jklymak commented Jan 26, 2022

tacaswell commented Feb 11, 2022

QuLogic left a comment

Choose a reason for hiding this comment

jklymak commented Feb 11, 2022

QuLogic commented Feb 11, 2022

jklymak commented Feb 12, 2022

jklymak Feb 21, 2022

Choose a reason for hiding this comment

jklymak commented Feb 21, 2022 • edited Loading

greglucas commented Feb 21, 2022

jklymak commented Feb 21, 2022

anntzer commented Feb 21, 2022

greglucas commented Feb 21, 2022

greglucas commented Mar 4, 2022

timhoffm commented Mar 8, 2022

lumberbot-app bot commented Mar 9, 2022

anntzer commented Apr 8, 2022

greglucas commented Apr 8, 2022

jklymak commented Jan 25, 2022 •

edited

Loading

timhoffm Jan 26, 2022 •

edited

Loading

jklymak commented Feb 21, 2022 •

edited

Loading