Repository navigation
Factor out common limits handling for x/y/z axes. - #21442
Conversation
f840d22 to
e463b79
Compare
| *xmax* respectively; *emit* and *auto* are used as is. | ||
| """ | ||
| v0name, v1name = names # The value names. | ||
| name, = [name for name, axis in self.axes._get_axis_map().items() |
There was a problem hiding this comment.
This seems a really obscure way to get the name to me. Is there some reason an axis can't know its "name" when it is created?
There was a problem hiding this comment.
Axises know their "name", but that's e.g. "r"/"theta" for polar axes whereas here we really want "x"/"y" even for polar. Perhaps Axises should have multiple name attributes for the various use cases, but that would be a bit hairy too...
jklymak
left a comment
jklymak
left a comment
There was a problem hiding this comment.
Overall 👍 - centralizing this makes sense, even if the root code is a little hared to parse, but the comments all help. Needs a rebase, and maybe a bit of thought about where "names" comes from - feel free to ping for a re-review.
|
Rebased. |
112c294 to
56cf49d
Compare
timhoffm
left a comment
timhoffm
left a comment
There was a problem hiding this comment.
May self-merge after adressing the one remaining comment.
| # rather than generically using set_view_interval, so that shared | ||
| # axes get updated as well. | ||
| raise NotImplementedError('Derived must override') | ||
| # docstring inherited |
There was a problem hiding this comment.
You can't have that comment and a docstring. :)
The change in set_top_view is necessary because super().set_xlim no
longer goes to 2D semantics (it's on the Axis.set_view_limits that one
would need to do a supercall), but in any case the old behavior was
actually more buggy because it would e.g. confuse data-limits sharex and
2D projection sharex; e.g.
`subplots(1, 2, sharex=True, subplot_kw={"projection": "3d"})`
was previously wrong.
As a side effect, also fixes `invert_yaxis`/`invert_zaxis` on 3d axises
(because they just go to the base `Axis.set_inverted`).
|
Doc failures are real. |
The "canonical" property names for Axes3D were switched from xlim3d/ylim3d to xlim/ylim so that Axes3D can directly inherit the setters from 2D Axes.
|
Test failure should be fixed now; foc failures seem related to the new scipy 1.8 release? |
|
This broke docs (see failed build in #22431): |
The change in set_top_view is necessary because super().set_xlim no
longer goes to 2D semantics (it's on the Axis.set_view_limits that one
would need to do a supercall), but in any case the old behavior was
actually more buggy because it would e.g. confuse data-limits sharex and
2D projection sharex; e.g.
subplots(1, 2, sharex=True, subplot_kw={"projection": "3d"})was previously wrong.
As a side effect, also fixes
invert_yaxis/invert_zaxison 3d axises (#21369)(because they just go to the base
Axis.set_inverted). Redoing the inheritancehierarchy (#21424) would probably(?) still be nice, at least from a semantic PoV.
old bad 3d sharex:
PR Summary
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand 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).