NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.4k

FIX: allow add option for Axes3D(fig)#19413

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

jklymak wants to merge3 commits intomatplotlib:masterfromjklymak:fix-3daxes

Closed

FIX: allow add option for Axes3D(fig)#19413

jklymak wants to merge3 commits intomatplotlib:masterfromjklymak:fix-3daxes

Conversation

Copy link

Member

jklymak commentedFeb 1, 2021•
edited
Loading

PR Summary

Closes#18939

We changed the logic to deal withAxes3d(fig) to not add the new axes tofig in#18564, but of course that broke some people who were working off older examples.

This PR pops up a DeprecationWarning if folks use the old call, however, if they do the somewhat awkward:

ax = Axes3d(fig)fig.add_axes(ax)

they willalso get a Deprecation Warning, which they can suppress with

ax = Axes3d(fig, add=False)fig.add_axes(ax)

However, the deprecation warning just suggests that they add the subplot the idiomatic way:

ax = fig.add_subplot(projection='3d')

I'm not a super fan of this proposal - it pops up a warning for a valid if rare use case, but its better than mysteriously not adding the Axes3d object. and we can rip it all out in a cycle or two.

PR Checklist

Has pytest style unit tests (andpytest passes).
IsFlake 8 compliant (runflake8 on changed files to check).
New features are documented, with examples if plot related.
Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
Conforms to Matplotlib style conventions (installflake8-docstrings and runflake8 --docstring-convention=all).
New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).

jklymak force-pushed thefix-3daxes branch frombb5d1cc to07cfa22Compare

February 1, 2021 06:47

anntzer added this to thev3.4.0 milestone

Feb 1, 2021

dstansby added the topic: mplot3d label

Feb 1, 2021

Copy link

MemberAuthor

jklymak commentedFeb 1, 2021

OK, and interestinglyAxes3D(fig), fig.add_axes(ax) isnot the same asfig.add_subplot(projection='3d'). So I'm not sure if there is a way to get this to work without asking for a kwarg.

Old

New

jklymak added the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label

Feb 1, 2021

Copy link

MemberAuthor

jklymak commentedFeb 1, 2021

(BTW Release Critical as the breaking change hasn't been released yet)

jklymak force-pushed thefix-3daxes branch from4cd420f to9a508c8Compare

February 1, 2021 17:11

Copy link

Member

WeatherGod commentedFeb 1, 2021

Just a historical note... originally, one created Axes3D by doingfig.add_axes(ax). This usually meant that the Axes3D object was not a "subplot" axes, and therefore, it did not get certain margins set. That is why the sizes of the plots are different between the two methods.

Copy link

MemberAuthor

jklymak commentedFeb 1, 2021

Yeah given the differences I'm still a little torn about how to do this.

Sticking add=False through our code isn't particularly nice.

Adding the axes twice is unacceptable.

Emitting a deprecation warning for something we think is acceptable is non optimal as well.

I guess another option is to have add_axes check if the same axes object is already on the stack and not add it again.

jklymak added3 commits

February 7, 2021 09:43

FIX: allow add option for Axes3D(fig)

24ac733

Fix

dcc9175

FIX: get tests working

31b6fe7

jklymak force-pushed thefix-3daxes branch from9a508c8 to31b6fe7Compare

February 7, 2021 17:44

Copy link

MemberAuthor

jklymak commentedFeb 7, 2021

Waiting for#19438

jklymak added the status: waiting for other PR label

Feb 7, 2021

Copy link

Member

tacaswell commentedFeb 10, 2021

Rather that touching the base classes__init__ can we spcial-case 3D in_process_projection_requirements so that only the Axes3D instances will see the extra kwarg?