Skip to content

FIX: clean up unit conversion unpacking of data, particularly for dates and pandas series #11664

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions lib/matplotlib/axes/_axes.py
Original file line number Diff line number Diff line change
Expand Up @@ -3901,10 +3901,10 @@ def dopatch(xs, ys, **kwargs):
N = len(bxpstats)
datashape_message = ("List of boxplot statistics and `{0}` "
"values must have same the length")
# check position
if positions is None:
positions = list(range(1, N + 1))
elif len(positions) != N:
positions = self.convert_xunits(positions)
if len(positions) != N:
raise ValueError(datashape_message.format("positions"))

# width
Expand Down Expand Up @@ -6048,7 +6048,7 @@ def pcolormesh(self, *args, alpha=None, norm=None, cmap=None, vmin=None,

# convert to one dimensional arrays
C = C.ravel()
coords = np.column_stack((X, Y)).astype(float, copy=False)
coords = np.column_stack((X, Y)).astype(float)
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any point in removing the "copy=False" kwarg. It saves time, and it is logically what is wanted here. A copy was already made via the column_stack call, so nothing done to coords will affect X or Y.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure. Changed back

collection = mcoll.QuadMesh(Nx - 1, Ny - 1, coords,
antialiased=antialiased, shading=shading,
**kwargs)
Expand Down
11 changes: 9 additions & 2 deletions lib/matplotlib/axis.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

import datetime
import logging
import numbers
import warnings

import numpy as np

Expand Down Expand Up @@ -1477,9 +1479,14 @@ def have_units(self):
return self.converter is not None or self.units is not None

def convert_units(self, x):
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be converting anything numlike, which includes things like pint.Quantity, into an array/masked array before calling convert. That seems like it would break pint, though I will note that our unit tests seem to be passing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great, I’ll check vs pint when I look at the other comments above. Thanks for the reminder...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I guess I still don't understand how all of this works. 😉 So is_numlike only returns true for things that are basic numbers (or contain them) and don't need converting. I think I'm ok with this code then. Tests passing is always a good thing too. 😄

# If x is already a number, doesn't need converting
# If x is already a number, doesn't need converting, but
# some iterables need to be massaged a bit...
if munits.ConversionInterface.is_numlike(x):
return x
if isinstance(x, list) or isinstance(x, numbers.Number):
Copy link
Member

Choose a reason for hiding this comment

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

What is the rationale for returning a list as-is instead of running it through np.asarray? Just that the latter is unnecessary? Or are there cases where the conversion would be an error?

return x
if issubclass(type(x), np.ma.MaskedArray):
Copy link
Member

Choose a reason for hiding this comment

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

Why is this using issubclass(type(x)... instead of isinstance?

return np.ma.asarray(x)
return np.asarray(x)
Copy link
Member

Choose a reason for hiding this comment

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

Is the point of this whole addition above to short-circuit some of what would otherwise happen in get_converter? I'm not opposed, just confused.

Copy link
Contributor

Choose a reason for hiding this comment

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

it seems worse, because calling np.asarray can drop information in some cases, making it unavailable in the call to convert below. Unless I'm missing something...


if self.converter is None:
self.converter = munits.registry.get_converter(x)
Expand Down
5 changes: 5 additions & 0 deletions lib/matplotlib/dates.py
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,11 @@ def date2num(d):
return _dt64_to_ordinalf(d)
if not d.size:
return d
if hasattr(d, 'dtype'):
if ((isinstance(d, np.ndarray) and
np.issubdtype(d.dtype, np.datetime64)) or
isinstance(d, np.datetime64)):
return _dt64_to_ordinalf(d)
Copy link
Member

Choose a reason for hiding this comment

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

What case is this whole block covering? Again, I'm confused.

return _to_ordinalf_np_vectorized(d)


Expand Down
17 changes: 4 additions & 13 deletions lib/matplotlib/testing/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,17 +91,8 @@ def pd():
pd = pytest.importorskip('pandas')
try:
from pandas.plotting import (
register_matplotlib_converters as register)
deregister_matplotlib_converters as deregister)
deregister()
except ImportError:
from pandas.tseries.converter import register
register()
try:
yield pd
finally:
try:
from pandas.plotting import (
deregister_matplotlib_converters as deregister)
except ImportError:
pass
else:
deregister()
pass
return pd
12 changes: 12 additions & 0 deletions lib/matplotlib/tests/test_axes.py
Original file line number Diff line number Diff line change
Expand Up @@ -5422,6 +5422,18 @@ def test_pandas_bar_align_center(pd):
fig.canvas.draw()


def test_pandas_data_unpack(pd):
# smoke test for all these different calling methods.
dates = [datetime.datetime(2018, 7, i) for i in range(1, 5)]
values = np.cumsum(np.random.rand(len(dates)))
df = pd.DataFrame({"dates": dates, "values": values})
plt.plot(df["dates"].values, df["values"])
plt.scatter(df["dates"], df["values"])
plt.plot("dates", "values", data=df)
plt.scatter(x="dates", y="values", data=df)
plt.draw()


def test_axis_set_tick_params_labelsize_labelcolor():
# Tests fix for issue 4346
axis_1 = plt.subplot()
Expand Down
10 changes: 10 additions & 0 deletions lib/matplotlib/tests/test_dates.py
Original file line number Diff line number Diff line change
Expand Up @@ -616,6 +616,16 @@ def tz_convert(*args):
_test_date2num_dst(pd.date_range, tz_convert)


def test_dateboxplot_pandas(pd):
# smoke test that this doesn't fail.
data = np.random.rand(5, 2)
years = pd.date_range('1/1/2000',
periods=2, freq=pd.DateOffset(years=1)).year
# Does not work
Copy link
Member

Choose a reason for hiding this comment

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

Another place I am confused. Is the comment obsolete?

plt.figure()
plt.boxplot(data, positions=years)


def _test_rrulewrapper(attach_tz, get_tz):
SYD = get_tz('Australia/Sydney')

Expand Down