FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

FIX: (broken)bar(h) math before units by jklymak · Pull Request #12903 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

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
anntzer merged 5 commits into from
Jan 13, 2019
Conversation 57 Commits 5 Checks 0 Files changed
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 .py  (2) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
84 changes: 73 additions & 11 deletions lib/matplotlib/axes/_axes.py
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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:

anntzer Jan 5, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality 
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.)

timhoffm Jan 5, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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]

anntzer Jan 6, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

doesnt work I think? convert_dx needs the converted x.

timhoffm Jan 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

jklymak Jan 7, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

anntzer Jan 7, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

jklymak Jan 7, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

... 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.

anntzer Jan 7, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ah, fair enough

timhoffm Jan 7, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

jklymak Jan 7, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

.... 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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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
Back | FazBrowse Home | New Git URL