10000 FIX: (broken)bar(h) math before units by jklymak · Pull Request #12903 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
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
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
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
55 changes: 43 additions & 12 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,26 @@ 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, x, convert):
"""
Small helper to do logic of width conversion flexibly.

*dx* and *x0* have units, but *x* has already been converted
to unitless. This allows the *dx* to have units that are
different from *x0*, but are still accepted by the ``__add__``
operator of *x0*.
"""
try:
# attempt to add the width to x0; this works for
# datetime+timedelta, for instance
dx = convert(x0 + dx) - x
except (TypeError, AttributeError):
# 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 @@ -2172,23 +2192,25 @@ def bar(self, x, height, width=0.8, bottom=None, *, align="center",
else:
raise ValueError('invalid orientation: %s' % orientation)

x, height, width, y, linewidth = np.broadcast_arrays(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this go below unit conversion? Otherwise I think units will be dropped if width is a list of unitized stuff?

Copy link
Member Author

Choose a reason for hiding this comment

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

y = [1 ,2, 4]
w = [timedelta(hours=2), timedelta(hours=3), timedelta(hours=3)]

print(np.broadcast_arrays(y, w))

I don't think this drops units.

[array([1, 2, 4]), array([datetime.timedelta(seconds=7200),
       datetime.timedelta(seconds=10800),
       datetime.timedelta(seconds=10800)], dtype=object)]

Copy link
Contributor

Choose a reason for hiding this comment

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

This will drop stuff like pint units or astropy units.
I remember some discussion a while ago which basically reached the conclusion that we cannot call any numpy function before unit handling (tbh I was rather unhappy about that, but the goal is not to relitigate this here), perhaps @dopplershift will remember this better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can someone provide an example? This works fine for all the categorical, unit, and date tests. If we are supporting something that doesn’t survive broadcasting we need to start testing it.

I’m not sure why we broadcast here at all anyway.

Copy link
Member

Choose a reason for hiding this comment

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

AFAICS, broadcasting is used to support scalar parameters such as plt.bar([1, 2, 3], [3,5,2], width=0.5)

Copy link
Contributor

Choose a reason for hiding this comment

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

For the record, broadcast_arrays will drop pint units--it might possibly work with astropy since it's an ndarray subclass. Broadcasting itself works fine, it's calling functions that need to allocate new arrays that's the problem; there are currently no hooks to allow overriding what type to use when allocating an empty array.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed - sorry, should have responded here - I did move the broadcast after the convert...

# Make args iterable too.
np.atleast_1d(x), height, width, y, linewidth)

# lets do some conversions now since some types cannot be
# subtracted uniformly
if self.xaxis is not None:
x0 = x
x = self.convert_xunits(x)
width = self.convert_xunits(width)
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:
y0 = y
y = self.convert_yunits(y)
height = self.convert_yunits(height)
height = self._convert_dx(height, y0, y, self.convert_yunits)
if yerr is not None:
yerr = self.convert_yunits(yerr)

x, height, width, y, linewidth = np.broadcast_arrays(
# Make args iterable too.
np.atleast_1d(x), height, width, y, linewidth)
yerr = self._convert_dx(yerr, y0, y, self.convert_yunits)

# Now that units have been converted, set the tick locations.
if orientation == 'vertical':
Expand Down Expand Up @@ -2465,10 +2487,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 = 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
0