Skip to content

FIX: remove colorbar from list of colorbars on axes #20980

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 9, 2021
Merged

FIX: remove colorbar from list of colorbars on axes #20980

merged 2 commits into from
Sep 9, 2021

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Sep 2, 2021

Closes #20978

If a colorbar was removed it was not being removed from a list of colorbars associated with an axes. This would confuse constrained_layout, as in #20978

test:

This now works...

import matplotlib.pyplot as plt
import numpy as np

figure, axes = plt.subplots(1, 1, constrained_layout=True)

data = np.random.normal(size=(10,10))
im = axes.imshow(data)
colorbar = figure.colorbar(im, ax=axes)
colorbar.remove()

plt.show()

@jklymak jklymak added this to the v3.5.0 milestone Sep 2, 2021
@jklymak jklymak mentioned this pull request Sep 2, 2021
Closed
@QuLogic QuLogic modified the milestones: v3.5.0, v3.4.4 Sep 2, 2021
@QuLogic
Copy link
Member

QuLogic commented Sep 2, 2021 •
edited
Loading

I think you might have meant to squash those two commits?

@jklymak
Copy link
Member Author

jklymak commented Sep 3, 2021

Yes, squashed and new test added as a separate commit.

greglucas
greglucas approved these changes Sep 3, 2021
View reviewed changes
lib/matplotlib/tests/test_colorbar.py
post_position = ax.get_position()
np.testing.assert_allclose(pre_position.get_points(),
post_position.get_points())

Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly add an assert that cb.ax isn't in the ax._colorbars list since that is the addition you're adding above?

Copy link
Member Author

Choose a reason for hiding this comment

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

My understanding is that we don't usually check private attributes?

Copy link
Member Author

Choose a reason for hiding this comment

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

If cb.ax is still in ax._colorbars this test will raise with the AttributeError: 'NoneType' object has no attribute 'transSubfigure' of the original issue. Thats what this test checks, I'm not convinced we need to test the implementation.

@timhoffm timhoffm merged commit d7b2a8d into matplotlib:master Sep 9, 2021
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Sep 9, 2021
@timhoffm @meeseeksmachine
Backport PR matplotlib#20980: FIX: remove colorbar from list of color…
dbb091a 
…bars on axes
@meeseeksmachine meeseeksmachine mentioned this pull request Sep 9, 2021
Merged
@lumberbot-app
Copy link

lumberbot-app bot commented Sep 9, 2021

Owee, I'm MrMeeseeks, Look at me.

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

  1. Checkout backport branch and update it.
$ git checkout v3.4.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 d7b2a8d4e1fda8d2d95a8cf3ed524f58c4a5fbaa
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #20980: FIX: remove colorbar from list of colorbars on axes'
  1. Push to a named branch :
git push YOURFORK v3.4.x:auto-backport-of-pr-20980-on-v3.4.x
  1. Create a PR against branch v3.4.x, I would have named this PR:

"Backport PR #20980 on branch v3.4.x (FIX: remove colorbar from list of colorbars on axes)"

And apply the correct labels and milestones.

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

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

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

timhoffm added a commit that referenced this pull request Sep 9, 2021
@timhoffm
Merge pull request #21030 from meeseeksmachine/auto-backport-of-pr-20…
a22b676 
…980-on-v3.5.x

Backport PR #20980 on branch v3.5.x (FIX: remove colorbar from list of colorbars on axes)
tacaswell pushed a commit that referenced this pull request Oct 20, 2021
@timhoffm
Merge pull request #20980 from jklymak/fix-remove-cb-cl
788d322 
FIX: remove colorbar from list of colorbars on axes
@QuLogic QuLogic modified the milestones: v3.4.4, v3.5.0 Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timhoffm timhoffm timhoffm approved these changes

@dstansby dstansby dstansby approved these changes

@greglucas greglucas greglucas approved these changes

Assignees
No one assigned
Labels
topic: color/colorbar
Projects
None yet
Milestone
v3.5.0
Development

Successfully merging this pull request may close these issues.

[Bug]: Error when removing colorbar in constrained layout
5 participants
@jklymak @QuLogic @timhoffm @dstansby @greglucas