Skip to content
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

BUG: Add Figure.delcolorbar() to fully delete a colorbar #3110

Merged
merged 1 commit into from Jun 11, 2014

Conversation

KevKeating
Copy link
Contributor

Closes #2688.

This creates a new Figure.delcolorbar() function, named to mimic Figure.delaxes(). The new function will delete a colorbar. If that colorbar was creating with use_gridspec=True, the function will also reset the gridspec. I added two tests to confirm that the function works as intended for colorbars creating with use_gridspec set to either True or False.

@tacaswell tacaswell added this to the v1.4.0 milestone Jun 3, 2014
@tacaswell
Copy link
Member

This probably does not need a image just, just ask the axes how big it is.

The colorbar to delete
"""

self.delaxes(cb.ax)
Copy link
Member

Choose a reason for hiding this comment

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

What happen in figures with out a colorbar? Sorry, being dumb.

@KevKeating
Copy link
Contributor Author

The new diff switches from Figure.delcolorbar to Colorbar.remove_from_figure. Also, the tests now compare axes.figbox rather than using image comparison (based on @tacaswell's suggestion). I also added a note to the changelog.

assert (pre_figbox == post_figbox).all()


def test_remove_from_figure_with_gridspec():
Copy link
Member

Choose a reason for hiding this comment

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

Both tests need and @cleanup or we get strange effects with tests cross-talking.

@tacaswell
Copy link
Member

Could you please squash this down to a single commit? (so we don't have the addition and removal in our repo forever)

@efiring
Copy link
Member

efiring commented Jun 4, 2014

Artists have a remove() method. The colorbar is not implemented as an Artist subclass, but wouldn't it still be better to give it a simply-named remove() method? And maybe Axes could also have a remove() method that would call self.get_figure().delaxes(self)? I don't like all this circularity, but it might be worth it for the sake of being able to invoke a remove method on just about anything that was added to a figure. The fewer method names a user needs to remember, the better.

@KevKeating
Copy link
Contributor Author

I prefer remove to remove_from_figure since there shouldn't be any ambiguity about what the colorbar is going to be removed from, but I'm okay with either option.

@KevKeating
Copy link
Contributor Author

I put up a new commit that should address all of the raised issues. I've changed the method name from remove_from_figure to remove, but I didn't add an Axes.remove method.

@efiring
Copy link
Member

efiring commented Jun 5, 2014

The Travis failure is unrelated.

@efiring
Copy link
Member

efiring commented Jun 5, 2014

Colorbar.colorbar_factory is normally used to make a colorbar (it is called by Figure.colorbar), and it performs two more operations that I think need to be undone when the colorbar is removed:

    mappable.callbacksSM.connect('changed', cb.on_mappable_changed)
    mappable.colorbar = cb

Also, when use_gridspec is False, it is still possible to undo the change that was made to the parent Axes. Something like this, where ax is the parent Axes:

pos = ax.get_position(original=True)
ax.set_position(pos)

@KevKeating
Copy link
Contributor Author

With the new commit, the callback and colorbar attribute are reset, and get_position/set_position are called when use_gridspec was False.

@tacaswell
Copy link
Member

This looks good to merge to me 👍

Thanks for fixing what was a very long standing bug.

efiring added a commit that referenced this pull request Jun 11, 2014
BUG: Add Figure.delcolorbar() to fully delete a colorbar
@efiring efiring merged commit bed37f7 into matplotlib:master Jun 11, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deleting axis in matplotlib > v1.2.1 does not work similar to v1.1.1
6 participants