-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Merge ParasiteAxesAuxTransBase into ParasiteAxesBase. #18675
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
Conversation
14ad460
to
1f79f5b
Compare
@@ -83,7 +83,7 @@ def test_ParasiteAxesAuxTrans(): | |||
ax1 = SubplotHost(fig, 1, 3, i+1) | |||
fig.add_subplot(ax1) | |||
|
|||
ax2 = ParasiteAxesAuxTrans(ax1, IdentityTransform()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is test_ParasiteAxesAuxTrans
; I don't think it should be changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the functionality of ParasiteAxesAuxTrans was fully folded into ParasiteAxes, so it should still be tested -- just as part of ParasiteAxes. I guess I could rename the test to test_parasite_auxtrans?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But ParasiteAxesAuxTrans
has a second copy of all that code, so if it's not tested, then it could break separately from ParasiteAxes
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I see; parametrized the test to check both classes.
this revealed that I needed to keep the __init__
on the AuxTransBase class too, because otherwise the deprecation machinery grabs object.__init__
instead and wraps that to install the deprecation emitter.
a066844
to
bbede52
Compare
""" | ||
Add a parasite axes to this host. | ||
|
||
Despite this method's name, this should actually be thought of as an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably add add_parasite_axes()
in a followup PR and (soft-)deprecate get_aux_axes
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do this in another PR, as you say.
bbede52
to
35a5e97
Compare
Some tests must be adapted for |
It doesn't warrant being a separate mixin class (cf. merging of Subplot into Axes -- flat is better than nested). Also, currently mpl_toolkits axes mixins create non-picklable Axes subclasses because they lack the same pickling machinery as the standard SubplotBase mixin. It's not too hard to also set up the same machinery for mpl_toolkits mixins (planned for a later PR), but also supporting the double-mixin case (both ParasiteAxesBase and ParasiteAxesAuxTransBase) adds unnecessary complications.
35a5e97
to
da2f77a
Compare
hopefully fixed now |
It doesn't warrant being a separate mixin class (cf. merging of Subplot
into Axes -- flat is better than nested).
Also, currently mpl_toolkits axes mixins create non-picklable Axes
subclasses because they lack the same pickling machinery as the
standard SubplotBase mixin. It's not too hard to also set up the
same machinery for mpl_toolkits mixins (planned for a later PR), but
also supporting the double-mixin case (both ParasiteAxesBase and
ParasiteAxesAuxTransBase) adds unnecessary complications.
PR Summary
PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
andpydocstyle<4
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).