Fix parasite_axes does not properly handle units #22789
Conversation
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
d653434 to
a622508
Compare
|
@oscargus What do I need to do in order to make this PR "workflow approved"? |
|
It is a default setting for new contributors. So once you have your first PR merged, all tests will run automatically for upcoming PRs. |
|
The label is just a reminder that this needs approval to run all tests. It doesn't affect anything else. |
|
Thank you @oscargus ! Could you or any one review this PR? |
| @@ -84,6 +85,43 @@ def test_twin_axes_empty_and_removed(): | |||
| plt.subplots_adjust(wspace=0.5, hspace=1) | |||
|
|
|||
|
|
|||
| @image_comparison(['twin_axes_both_with_units.png']) | |||
There was a problem hiding this comment.
You are changing the main library, but only testing the axes_grid toolkit. If this has ramifications for the normal Axes.twinx/y please test that as well, or even preferentially.
There was a problem hiding this comment.
We just added a new optional parameter to it, which should not affect the existing functionalities. All the regression tests for the main library have been passed as well.
|
Sorry, I do not have enough knowledge about this module to be able to review it. (Just helping out with the formalities.) |
| @@ -20,7 +21,8 @@ def __init__(self, parent_axes, aux_transform=None, | |||
| def cla(self): | |||
| super().cla() | |||
| martist.setp(self.get_children(), visible=False) | |||
| self._get_lines = self._parent_axes._get_lines | |||
| self._get_lines = functools.partial( | |||
There was a problem hiding this comment.
I don't understand this change - can you explain why you are doing this?
Right now parasite_axes just use self._parent_axes._get_lines as self._get_lines, but it can't update the axes unit when there are twin axes. Therefore, we need to provide axes=self as an extra args to handle this. We also need to change the callees to use axes in kwargs when provided. Add a unit test for missing unit in twin axes This test creates a plot with twin axes where both axes have units. It then checks whether units are appended correctly on the respective axes. The code base without the modification fails the unit test whereas the modification makes it pass the unit test.t:q`
a622508 to
96c6168
Compare
|
Took care of at least the rebase to keep up with changes in the library since this was opened. I'm a little skeptical about the mechanism of using an undocumented argument to sneakily replace an instance attribute (at call time). However, I do think I'm convinced that this is the most minimal change that addresses the problem at hand. It is at least only on an already private class. Essentially the crux of the matter is that the The most concrete thing that is kept as part of this object is the color cycle state. One alternative I explored, but think is fragile at best, was instead of replacing the The other direction that I think may be better, but haven't fully thought through is removing the |
|
@ksunden See also #19484 re: sharing of the color cycle state. I don't think it's immediately usable in parasite_axes because right now the prop_cycle needs to be "declared" as shareable from the very beginning, but that's probably fixable, and using that in parasite_axes may be cleaner here. On the other hand, your last suggestion (removing the axes reference from _process_plot_var_args) also seems to work well, i.e. the following patch seems good for me: diff --git i/lib/matplotlib/axes/_axes.py w/lib/matplotlib/axes/_axes.py
index a6fecd374f..a827654100 100644
--- i/lib/matplotlib/axes/_axes.py
+++ w/lib/matplotlib/axes/_axes.py
@@ -1685,7 +1685,7 @@ class Axes(_AxesBase):
(``'green'``) or hex strings (``'#008000'``).
"""
kwargs = cbook.normalize_kwargs(kwargs, mlines.Line2D)
- lines = [*self._get_lines(*args, data=data, **kwargs)]
+ lines = [*self._get_lines(self, *args, data=data, **kwargs)]
for line in lines:
self.add_line(line)
if scalex:
@@ -3546,7 +3546,8 @@ class Axes(_AxesBase):
# that would call self._process_unit_info again, and do other indirect
# data processing.
(data_line, base_style), = self._get_lines._plot_args(
- (x, y) if fmt == '' else (x, y, fmt), kwargs, return_kwargs=True)
+ self, (x, y) if fmt == '' else (x, y, fmt), kwargs,
+ return_kwargs=True)
# Do this after creating `data_line` to avoid modifying `base_style`.
if barsabove:
@@ -5229,7 +5230,8 @@ default: :rc:`scatter.edgecolors`
# For compatibility(!), get aliases from Line2D rather than Patch.
kwargs = cbook.normalize_kwargs(kwargs, mlines.Line2D)
# _get_patches_for_fill returns a generator, convert it to a list.
- patches = [*self._get_patches_for_fill(*args, data=data, **kwargs)]
+ patches = [
+ *self._get_patches_for_fill(self, *args, data=data, **kwargs)]
for poly in patches:
self.add_patch(poly)
self._request_autoscale_view()
diff --git i/lib/matplotlib/axes/_base.py w/lib/matplotlib/axes/_base.py
index 53f5607f41..52388a891b 100644
--- i/lib/matplotlib/axes/_base.py
+++ w/lib/matplotlib/axes/_base.py
@@ -219,14 +219,14 @@ class _process_plot_var_args:
an arbitrary number of *x*, *y*, *fmt* are allowed
"""
- def __init__(self, axes, command='plot'):
- self.axes = axes
+
+ def __init__(self, command):
self.command = command
self.set_prop_cycle(None)
def __getstate__(self):
# note: it is not possible to pickle a generator (and thus a cycler).
- return {'axes': self.axes, 'command': self.command}
+ return {'command': self.command}
def __setstate__(self, state):
self.__dict__ = state.copy()
@@ -238,8 +238,8 @@ class _process_plot_var_args:
self.prop_cycler = itertools.cycle(cycler)
self._prop_keys = cycler.keys # This should make a copy
- def __call__(self, *args, data=None, **kwargs):
- self.axes._process_unit_info(kwargs=kwargs)
+ def __call__(self, axes, *args, data=None, **kwargs):
+ axes._process_unit_info(kwargs=kwargs)
for pos_only in "xy":
if pos_only in kwargs:
@@ -309,7 +309,8 @@ class _process_plot_var_args:
this += args[0],
args = args[1:]
yield from self._plot_args(
- this, kwargs, ambiguous_fmt_datakey=ambiguous_fmt_datakey)
+ axes, this, kwargs,
+ ambiguous_fmt_datakey=ambiguous_fmt_datakey)
def get_next_color(self):
"""Return the next color in the cycle."""
@@ -344,17 +345,17 @@ class _process_plot_var_args:
if kw.get(k, None) is None:
kw[k] = defaults[k]
- def _makeline(self, x, y, kw, kwargs):
+ def _makeline(self, axes, x, y, kw, kwargs):
kw = {**kw, **kwargs} # Don't modify the original kw.
default_dict = self._getdefaults(set(), kw)
self._setdefaults(default_dict, kw)
seg = mlines.Line2D(x, y, **kw)
return seg, kw
- def _makefill(self, x, y, kw, kwargs):
+ def _makefill(self, axes, x, y, kw, kwargs):
# Polygon doesn't directly support unitized inputs.
- x = self.axes.convert_xunits(x)
- y = self.axes.convert_yunits(y)
+ x = axes.convert_xunits(x)
+ y = axes.convert_yunits(y)
kw = kw.copy() # Don't modify the original kw.
kwargs = kwargs.copy()
@@ -403,7 +404,7 @@ class _process_plot_var_args:
seg.set(**kwargs)
return seg, kwargs
- def _plot_args(self, tup, kwargs, *,
+ def _plot_args(self, axes, tup, kwargs, *,
return_kwargs=False, ambiguous_fmt_datakey=False):
"""
Process the arguments of ``plot([x], y, [fmt], **kwargs)`` calls.
@@ -495,10 +496,10 @@ class _process_plot_var_args:
else:
x, y = index_of(xy[-1])
- if self.axes.xaxis is not None:
- self.axes.xaxis.update_units(x)
- if self.axes.yaxis is not None:
- self.axes.yaxis.update_units(y)
+ if axes.xaxis is not None:
+ axes.xaxis.update_units(x)
+ if axes.yaxis is not None:
+ axes.yaxis.update_units(y)
if x.shape[0] != y.shape[0]:
raise ValueError(f"x and y must have same first dimension, but "
@@ -534,7 +535,7 @@ class _process_plot_var_args:
else:
labels = [label] * n_datasets
- result = (make_artist(x[:, j % ncx], y[:, j % ncy], kw,
+ result = (make_artist(axes, x[:, j % ncx], y[:, j % ncy], kw,
{**kwargs, 'label': label})
for j, label in enumerate(labels))
@@ -1295,8 +1296,8 @@ class _AxesBase(martist.Artist):
self._tight = None
self._use_sticky_edges = True
- self._get_lines = _process_plot_var_args(self)
- self._get_patches_for_fill = _process_plot_var_args(self, 'fill')
+ self._get_lines = _process_plot_var_args('plot')
+ self._get_patches_for_fill = _process_plot_var_args('fill')
self._gridOn = mpl.rcParams['axes.grid']
old_children, self._children = self._children, []
diff --git i/lib/mpl_toolkits/mplot3d/axes3d.py w/lib/mpl_toolkits/mplot3d/axes3d.py
index f9a4813628..a529bbc3a3 100644
--- i/lib/mpl_toolkits/mplot3d/axes3d.py
+++ w/lib/mpl_toolkits/mplot3d/axes3d.py
@@ -2993,7 +2993,8 @@ class Axes3D(Axes):
# that would call self._process_unit_info again, and do other indirect
# data processing.
(data_line, base_style), = self._get_lines._plot_args(
- (x, y) if fmt == '' else (x, y, fmt), kwargs, return_kwargs=True)
+ axes, (x, y) if fmt == '' else (x, y, fmt), kwargs,
+ return_kwargs=True)
art3d.line_2d_to_3d(data_line, zs=z)
# Do this after creating `data_line` to avoid modifying `base_style`. |
|
Moved to draft because I think this is not fully ready and the diff proposed above by @anntzer may prove to be a better route. |
|
Superseded by #26078. Thanks for the PR! |
PR Summary
Right now parasite_axes just use self._parent_axes._get_lines as self._get_lines, but it can't update the axes unit when there are twin axes. Therefore, we need to provide axes=self as an extra args to handle this. We also need to change the callees to use axes in kwargs when provided.
This new added test creates a plot with twin axes where both axes have units. It then checks whether units are appended correctly on the respective axes. The code base without the modification fails the unit test whereas the modification makes it pass the unit test.
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).Fixes #22714