Skip to content

API: treat xunits=None and yunits=None as "default" #17828

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.

Sign up for GitHub

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
Jul 5, 2020
Merged

API: treat xunits=None and yunits=None as "default" #17828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions doc/api/next_api_changes/behavior/17828-TAC.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
xunits=None and yunits=None passed as kwargs are treated as "no action"
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Many (but not all) of the methods on `~.axes.Axes` take the (undocumented)
kwargs *xunits* and *yunits* that will update the units on the given
Axis by calling `.Axis.set_units` and `.Axis.update_units`.

Previously if `None` was passed it would clear the value stored in
``.Axis.units`` which will in turn break converters (notably
`.StrCategoryConverter`) which rely on the value in
``.Axis.units`` to work properly.

This changes the semantics of ``ax.meth(..., xunits=None,
yunits=None)`` from "please clear the units" to "do the default thing
as if they had not been passed" which is consistent with the standard
behavior of Matplotlib keyword arguments.

If you were relying on passing ``xuints=None`` to plotting methods to
clear the ``.Axes.units`` attribute, directly call `.Axis.set_units` (and
`.Axis.update_units` if you also require the converter to be updated).
16 changes: 13 additions & 3 deletions lib/matplotlib/axes/_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2172,7 +2172,15 @@ def update_datalim_bounds(self, bounds):
self.dataLim.set(mtransforms.Bbox.union([self.dataLim, bounds]))

def _process_unit_info(self, xdata=None, ydata=None, kwargs=None):
"""Look for unit *kwargs* and update the axis instances as necessary"""
"""
Look for unit *kwargs* and update the axis instances as necessary


.. warning ::

This method may mutate the dictionary passed in an kwargs and
the Axis instances attached to this Axes.
"""

def _process_single_axis(data, axis, unit_name, kwargs):
# Return if there's no axis set
Expand All @@ -2188,10 +2196,12 @@ def _process_single_axis(data, axis, unit_name, kwargs):
if kwargs is not None:
units = kwargs.pop(unit_name, axis.units)
if self.name == 'polar':
# handle special casing to allow the kwargs
# thetaunits and runits to be used with polar
polar_units = {'xunits': 'thetaunits', 'yunits': 'runits'}
units = kwargs.pop(polar_units[unit_name], units)

if units != axis.units:
if units != axis.units and units is not None:
axis.set_units(units)
# If the units being set imply a different converter,
# we need to update.
Expand Down Expand Up @@ -3780,7 +3790,7 @@ def set_navigate_mode(self, b):
"""
Set the navigation toolbar button status;

.. warning::
.. warning ::
this is not a user-API function.

"""
Expand Down
26 changes: 26 additions & 0 deletions lib/matplotlib/tests/test_category.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from matplotlib.axes import Axes
import matplotlib.pyplot as plt
import matplotlib.category as cat
from matplotlib.testing.decorators import check_figures_equal


class TestUnitData:
Expand Down Expand Up @@ -269,3 +270,28 @@ def test_mixed_type_update_exception(self, ax, plotter, xdata):
with pytest.raises(TypeError):
plotter(ax, [0, 3], [1, 3])
plotter(ax, xdata, [1, 2])


@pytest.mark.style('default')
@check_figures_equal(extensions=["png"])
def test_overriding_units_in_plot(fig_test, fig_ref):
from datetime import datetime

t0 = datetime(2018, 3, 1)
t1 = datetime(2018, 3, 2)
t2 = datetime(2018, 3, 3)
t3 = datetime(2018, 3, 4)

Comment on lines +278 to +284
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the point of adding in the datetime here if that's not what's being tested?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a converter class that does not care about the value of Axis.units, but I think it is worth putting units on both Axis.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to test using some data that does care about/use units? I'm wondering if datetime always has the units set to None all the time anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The string catarogicals do use Axis.units to store critical state (which is what broke in #11095).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh woops, I missed that categoricals were being plotted too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tacaswell so that's what concerns me, that you're putting two potential points of failure in this test - when it fails will it be easy to see that it was the categoricals that broke this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of our tests as pretty much closer to integration tests than unit tests. I am optimistic that if (when) this break who ever is debugging it will be able to sort out the root cause with out too much trouble.

ax_test = fig_test.subplots()
ax_ref = fig_ref.subplots()
for ax, kwargs in zip([ax_test, ax_ref],
({}, dict(xunits=None, yunits=None))):
# First call works
ax.plot([t0, t1], ["V1", "V2"], **kwargs)
x_units = ax.xaxis.units
y_units = ax.yaxis.units
# this should not raise
ax.plot([t2, t3], ["V1", "V2"], **kwargs)
# assert that we have not re-set the units attribute at all
assert x_units is ax.xaxis.units
assert y_units is ax.yaxis.units