Skip to content

Cleanup delaxes() #9928

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

Merged
merged 1 commit into from
Dec 5, 2017
Merged

Cleanup delaxes() #9928

merged 1 commit into from
Dec 5, 2017

Conversation

timhoffm
Copy link
Member

@timhoffm timhoffm commented Dec 5, 2017

This cleans up the delaxes() code and docstring as discussed in #9912.

For all intended uses, the API compatibility is preserved. Unreasonable and possibly wrong uses like multiple positional arguments or invalid keyword arguments will now raise an Exception.

Note: So far, Figure.delaxes() raises a KeyError if ax does not exist. IMO a ValueError would be more appropriate. However, I've not touched this beacuse of API compatibility. Please report back if I should turn the KeyError into a ValueError.

@efiring
Copy link
Member

efiring commented Dec 5, 2017

I agree about the most appropriate exception to raise, but it's not critical, and probably best to defer that sort of API-breaking cleanup. I approve, and will merge this as-is.

@efiring efiring merged commit 2f28286 into matplotlib:master Dec 5, 2017
@timhoffm timhoffm deleted the cleanup-delaxes branch December 5, 2017 03:09
@lkjell
Copy link
Contributor

lkjell commented Dec 5, 2017

Is it cheaper to check if self.axes is not empty than creating a new axes instance with gca if there is no axes exist?

@timhoffm
Copy link
Member Author

timhoffm commented Dec 5, 2017

@lkjell Are you referring to plt.delaxes() when the figure does not have an axes? That's indeed an unnecessary axes creation and deletion. However, you'll most likely not trying to delete axes from empty figures a lot, so the performance penality is negligible. I've not changed the algorithmic behavior in this PR, just code and docstring cleanup.

@lkjell
Copy link
Contributor

lkjell commented Dec 5, 2017

I do. Not sure why implicit deletion does not return the deleted axes. With plt.delaxes there is no guard to undo the deletion.

@timhoffm
Copy link
Member Author

timhoffm commented Dec 5, 2017

Figure.del_axes does not return the deleted axes. So i removed the return value to not make any false promises. It has always been None anyway.

That said, a concept of "undo" is quite uncommon in programming, even if it's interactive. I wouldn't have expected this to be possible.

@tacaswell
Copy link
Member

I would not worry about the speed in the case where someone is using plt.delaxes(), it is probably interactive usage where they are typing and the speed difference will be tiny compared to the speed at which they are typing.

If the user passes in the axes to delete they are already holding it. If it is implicit, then the assumption is the user is interactive and does not care.

@tacaswell tacaswell added this to the v2.2 milestone Dec 6, 2017
@QuLogic QuLogic modified the milestones: needs sorting, v2.2.0 Feb 12, 2018
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.

6 participants