Skip to content

FIX: (broken)bar(h) math before units #12903

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 5 commits into from
Jan 13, 2019
+108 −11
Merged

FIX: (broken)bar(h) math before units #12903

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1dfdc29
FIX: brokenbarh math before units
jklymak Nov 27, 2018
7c4b000
FIX: make robust to units
jklymak Jan 12, 2019
aa6dfaa
FIX: make robust to units
jklymak Jan 12, 2019
6e410db
FIX: make robust to units
jklymak Jan 12, 2019
f9c43b7
FIX: make robust to units
jklymak Jan 12, 2019
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
84 changes: 73 additions & 11 deletions lib/matplotlib/axes/_axes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2011,6 +2011,58 @@ def step(self, x, y, *args, where='pre', data=None, **kwargs):
kwargs['drawstyle'] = 'steps-' + where
return self.plot(x, y, *args, data=data, **kwargs)

@staticmethod
def _convert_dx(dx, x0, xconv, convert):
"""
Small helper to do logic of width conversion flexibly.

*dx* and *x0* have units, but *xconv* has already been converted
to unitless (and is an ndarray). This allows the *dx* to have units
that are different from *x0*, but are still accepted by the
``__add__`` operator of *x0*.
"""

# x should be an array...
assert type(xconv) is np.ndarray

if xconv.size == 0:
# xconv has already been converted, but maybe empty...
return convert(dx)

try:
# attempt to add the width to x0; this works for
# datetime+timedelta, for instance

# only use the first element of x and x0. This saves
# having to be sure addition works across the whole
# vector. This is particularly an issue if
# x0 and dx are lists so x0 + dx just concatenates the lists.
# We can't just cast x0 and dx to numpy arrays because that
# removes the units from unit packages like `pint` that
# wrap numpy arrays.
try:
x0 = x0[0]
except (TypeError, IndexError, KeyError):
x0 = x0

try:
x = xconv[0]
except (TypeError, IndexError, KeyError):
x = xconv

delist = False
if not np.iterable(dx):
dx = [dx]
delist = True
dx = [convert(x0 + ddx) - x for ddx in dx]
if delist:
dx = dx[0]
except (TypeError, AttributeError) as e:
# but doesn't work for 'string' + float, so just
# see if the converter works on the float.
dx = convert(dx)
return dx

@_preprocess_data()
@docstring.dedent_interpd
def bar(self, x, height, width=0.8, bottom=None, *, align="center",
Expand Down Expand Up @@ -2175,16 +2227,17 @@ def bar(self, x, height, width=0.8, bottom=None, *, align="center",
# lets do some conversions now since some types cannot be
# subtracted uniformly
if self.xaxis is not None:
x = self.convert_xunits(x)
width = self.convert_xunits(width)
x0 = x
x = np.asarray(self.convert_xunits(x))
width = self._convert_dx(width, x0, x, self.convert_xunits)
if xerr is not None:
xerr = self.convert_xunits(xerr)

xerr = self._convert_dx(xerr, x0, x, self.convert_xunits)
if self.yaxis is not None:
y = self.convert_yunits(y)
height = self.convert_yunits(height)
y0 = y
y = np.asarray(self.convert_yunits(y))
height = self._convert_dx(height, y0, y, self.convert_yunits)
if yerr is not None:
yerr = self.convert_yunits(yerr)
yerr = self._convert_dx(yerr, y0, y, self.convert_yunits)

x, height, width, y, linewidth = np.broadcast_arrays(
# Make args iterable too.
Expand Down Expand Up @@ -2465,10 +2518,19 @@ def broken_barh(self, xranges, yrange, **kwargs):
self._process_unit_info(xdata=xdata,
ydata=ydata,
kwargs=kwargs)
xranges = self.convert_xunits(xranges)
yrange = self.convert_yunits(yrange)

col = mcoll.BrokenBarHCollection(xranges, yrange, **kwargs)
xranges_conv = []
for xr in xranges:
Copy link
Contributor

Choose a reason for hiding this comment

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

for x, dx in xranges:
    x_conv = self.convert_xunits(x)
    dx_conv = self._convert_dx(...)
    xranges_conv.append((...))

(Note that your current implementation will silently work with xranges items with more than 2 elements, silently dropping the extra ones.)

Copy link
Member

Choose a reason for hiding this comment

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

Even simpler with a list comprehension:

xranges_conv = [(self.convert_xunits(x),
                 self._convert_dx(x, x0, dx, self.convert_xunits))
                for x, dx in xranges]

Copy link
Contributor

Choose a reason for hiding this comment

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

doesnt work I think? convert_dx needs the converted x.

Copy link
Member

Choose a reason for hiding this comment

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

True. Thanks for spotting the error.

Either go back to using a loop, or change _convert_dx to _convert_x_dx, in which case I probably would use the order _convert_x_dx(x, dx, x0, ...) because x0 is only a helper.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahem, OK, reverted...

@anntzer I don't see whats wrong w/ silently dropping extra stuff. Is that worse than throwing an explicit but mysterious error?

Copy link
Contributor

Choose a reason for hiding this comment

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

But then someone tries to directly build a BrokenBarHCollection with invalid data, and gets an unclear error message, and asks for validation to be implemented there again, and we end up with duplicated validation checks and error messages.
It's not really hard to go up once in the stack trace?

Copy link
Member Author

Choose a reason for hiding this comment

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

... But we need the validation here, as you just pointed out above? But you want to do it implictly, by trying to unpack the tuple, which would also yield a ValueError, but more unhelpfully: ValueError: too many values to unpack (expected 2). This is just providing a more helpful error.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, fair enough

Copy link
Member

Choose a reason for hiding this comment

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

As a user, I prefer errors to be raised in the routine I called rather than in something I had no idea existed...

raise ... from ... was introduced for this case.

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 what that is and google wasn’t particularly helpful. How would this get used in this case?

if len(xr) != 2:
raise ValueError('each range in xrange must be a sequence '
'with two elements (i.e. an Nx2 array)')
# convert the absolute values, not the x and dx...
x_conv = np.asarray(self.convert_xunits(xr[0]))
x1 = self._convert_dx(xr[1], xr[0], x_conv, self.convert_xunits)
xranges_conv.append((x_conv, x1))

yrange_conv = self.convert_yunits(yrange)

col = mcoll.BrokenBarHCollection(xranges_conv, yrange_conv, **kwargs)
self.add_collection(col, autolim=True)
self.autoscale_view()

Expand Down
35 changes: 35 additions & 0 deletions lib/matplotlib/tests/test_axes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1498,6 +1498,32 @@ def test_barh_tick_label():
align='center')


def test_bar_timedelta():
"""smoketest that bar can handle width and height in delta units"""
fig, ax = plt.subplots()
ax.bar(datetime.datetime(2018, 1, 1), 1.,
width=datetime.timedelta(hours=3))
ax.bar(datetime.datetime(2018, 1, 1), 1.,
xerr=datetime.timedelta(hours=2),
width=datetime.timedelta(hours=3))
fig, ax = plt.subplots()
ax.barh(datetime.datetime(2018, 1, 1), 1,
height=datetime.timedelta(hours=3))
ax.barh(datetime.datetime(2018, 1, 1), 1,
height=datetime.timedelta(hours=3),
yerr=datetime.timedelta(hours=2))
fig, ax = plt.subplots()
ax.barh([datetime.datetime(2018, 1, 1), datetime.datetime(2018, 1, 1)],
np.array([1, 1.5]),
height=datetime.timedelta(hours=3))
ax.barh([datetime.datetime(2018, 1, 1), datetime.datetime(2018, 1, 1)],
np.array([1, 1.5]),
height=[datetime.timedelta(hours=t) for t in [1, 2]])
ax.broken_barh([(datetime.datetime(2018, 1, 1),
datetime.timedelta(hours=1))],
(10, 20))


@image_comparison(baseline_images=['hist_log'],
remove_text=True)
def test_hist_log():
Expand Down Expand Up @@ -5433,6 +5459,15 @@ def test_broken_barh_empty():
ax.broken_barh([], (.1, .5))


def test_broken_barh_timedelta():
"""Check that timedelta works as x, dx pair for this method """
fig, ax = plt.subplots()
pp = ax.broken_barh([(datetime.datetime(2018, 11, 9, 0, 0, 0),
datetime.timedelta(hours=1))], [1, 2])
assert pp.get_paths()[0].vertices[0, 0] == 737007.0
assert pp.get_paths()[0].vertices[2, 0] == 737007.0 + 1 / 24


def test_pandas_pcolormesh(pd):
time = pd.date_range('2000-01-01', periods=10)
depth = np.arange(20)
Expand Down