Skip to content

Fix TypeError when plotting stacked bar chart with decimal #13738

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

Merged
merged 28 commits into from
May 12, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
e1985d7
Fix issue 10788
sasoripathos Mar 22, 2019
2279b16
Fix style
sasoripathos Mar 22, 2019
19bb0c7
Add missing import statement
sasoripathos Mar 22, 2019
05dd388
Move Decimal-float conversion to earlier point
sasoripathos Mar 24, 2019
61b491f
Fix typo
sasoripathos Mar 24, 2019
0a23d63
Convert Decimal in convert_units in Axis
sasoripathos Apr 6, 2019
b7fdc9b
Merge remote-tracking branch 'upstream/master'
sasoripathos Apr 6, 2019
4be1d99
Merge branch 'master' into fix10788
sasoripathos Apr 6, 2019
125dec7
Add an length check to prevent potential error
sasoripathos Apr 6, 2019
eda2d01
Use iterator to avoid keyerror
sasoripathos Apr 6, 2019
109f252
Merge remote-tracking branch 'upstream/master'
sasoripathos Apr 19, 2019
cb82df8
Merge branch 'master' into fix10788
sasoripathos Apr 19, 2019
7359ece
Add DecimalConverter in units.py
sasoripathos Apr 19, 2019
45da1c5
Don't perform retey in convert_units
sasoripathos Apr 24, 2019
b2ca609
Merge remote-tracking branch 'upstream/master'
sasoripathos Apr 24, 2019
e293653
Merge branch 'master' into fix10788
sasoripathos Apr 24, 2019
2c5a873
Merge remote-tracking branch 'upstream/master'
sasoripathos Apr 30, 2019
cb0a64c
Remove changes
sasoripathos Apr 30, 2019
b8ae7bd
Merge remote-tracking branch 'upstream/master'
sasoripathos May 1, 2019
124cc71
Resolve conflict, update comment
sasoripathos May 1, 2019
6e50219
Merge remote-tracking branch 'upstream/master'
sasoripathos May 7, 2019
8cbafae
Merge remote-tracking branch 'upstream/master'
sasoripathos May 8, 2019
b27afa3
Merge remote-tracking branch 'upstream/master'
sasoripathos May 11, 2019
ed456e4
Merge branch 'master' into fix10788
sasoripathos May 11, 2019
7d13788
Merge remote-tracking branch 'upstream/master' into fix10788
sasoripathos May 12, 2019
4005002
Add is_natively_supported in ConversionInterface
sasoripathos May 12, 2019
b290867
Add more test cases
sasoripathos May 12, 2019
c56c175
Fix missing self argument
sasoripathos May 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
4 changes: 2 additions & 2 deletions lib/matplotlib/axis.py
Original file line number Diff line number Diff line change
Expand Up @@ -1565,8 +1565,8 @@ def have_units(self):
return self.converter is not None or self.units is not None

def convert_units(self, x):
# If x is already a number, doesn't need converting
if munits.ConversionInterface.is_numlike(x):
Copy link
Member

@timhoffm timhoffm Apr 26, 2019

Choose a reason for hiding this comment

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

Is it really necessary this way? The purpose of converters is to convert to something number-like matplotlib can work with. So the original logic "if is number return else convert" seems more clear than "convert; if that fails, did we already have a number? Ok then we just use it".

Maybe munits.ConversionInterface.is_numlike() is just not the right function/name. I'd prefer this to stay and be

if not munits.ConversionInterface.needs_conversion(x):
    return x

That will allow to cleanly pass through Decimals.

Copy link
Member

Choose a reason for hiding this comment

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

Well, not sure on the sign of the logic is_natively_supported_numlike(x) may be better than not needs_conversion(x).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original cause of this problem is the operations like plus between Decimal and float is not supported by matplotlib. If there is not such operations in the logic, Decimal input will not cause any error. As we discussed before, it doesn't quite make sense to distinguish Decimal and other types of number since Decimal is a type of Number.

Copy link
Member

Choose a reason for hiding this comment

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

That's why I'm saying "is_numlike" is not the right check. Semantically, we want

if matplotlib_can_natively_work_with(x):
     return x
# try to convert x

It's not about number or not, it's about "do we need a converter". So, if matplotlib cannot natively handle Decimal that can fall through to the converter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can we define natively handle? For Decimal input, matplotlib supports it if there is no operation between Decimal and float. For example:

x = [1, 2, 3, 4]
y = [Decimal(1.5), Decimal(2.5), Decimal(3.5), Decimal(4.5)]
ax.bar(x, y, align='center')

This piece of code works fine. I am not sure about other kinds of plot, but based on this, I think matplotlib does not totally support/unsupport Decimal.

Copy link
Member

Choose a reason for hiding this comment

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

Well, operationally, "can natively handle" is currently is_numlike(x) and not isinstance(x, Decimal). I mean, that's what your proposed code does as well.

It's a bit clumsy to define is that way, but that's the current logic. And I prefer to have that logic explicitly written down in a single place in a function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see currently is_numlike is used in some places like StrCategoryConverter.convert() in category.py. And from the code (as below), it looks like the Decimal input will not cause any problems here (since Decimal will be converted to floats).

# pass through sequence of non binary numbers
if all((units.ConversionInterface.is_numlike(v) and
        not isinstance(v, (str, bytes))) for v in values):
    return np.asarray(values, dtype=float)

So I think maybe instead of change is_numlike(x), which cause API changes, we can add a new function is_natively_supported_numlike(x) like proposed in ConversionInterface. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's a good idea to leave is_numlike(x) as is and introduce and use ConversionInterface.is_natively_supported_numlike(x)

Naming: Not sure if we need the _numlike suffix in the name. On the one hand, this is exactly meant as is_natively_supported - no artificial restriction to numlikes necessary (even though it's unlikely that we'll natively support non-numlikes). On the other hand, the _numlike may give additional context to the reader, but I would see this more as a job of the is_natively_supported* docstring.

is_numlike(x) may become deprecated in the future, but we can handle this later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added a function named is_natively_supported() in ConversionInterface. Since the current unit system assume lists are homogeneous, I followed the same style as is_numlik(). Now I only consider all numbers except Decimal are natively supported.

# If x is natively supported by Matplotlib, doesn't need converting
if munits.ConversionInterface.is_natively_supported(x):
return x

if self.converter is None:
Expand Down
84 changes: 84 additions & 0 deletions lib/matplotlib/tests/test_axes.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import numpy as np
from numpy import ma
from cycler import cycler
from decimal import Decimal
import pytest

import warnings
Expand Down Expand Up @@ -1477,6 +1478,62 @@ def test_bar_tick_label_multiple_old_alignment():
align='center')


@check_figures_equal(extensions=["png"])
def test_bar_decimal_center(fig_test, fig_ref):
ax = fig_test.subplots()
Copy link
Member

Choose a reason for hiding this comment

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

Just for symmetry, please move this down to # Test image-

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @timhoffm for advice. Have moved it down.

x0 = [1.5, 8.4, 5.3, 4.2]
y0 = [1.1, 2.2, 3.3, 4.4]
x = [Decimal(x) for x in x0]
y = [Decimal(y) for y in y0]
# Test image - vertical, align-center bar chart with Decimal() input
ax.bar(x, y, align='center')
# Reference image
ax = fig_ref.subplots()
ax.bar(x0, y0, align='center')


@check_figures_equal(extensions=["png"])
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think testing both bar and barh is particularly necessary and would just keep one of them.

Copy link
Member

Choose a reason for hiding this comment

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

what happens if width (or height) is a Decimal?

Copy link
Contributor Author

@sasoripathos sasoripathos May 12, 2019

Choose a reason for hiding this comment

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

I think there will be no problem since width will also be converted. In the following code, if width is of type Decimal, it will be converted to floats by convert_xunits in _convert_dx eventually.

 if self.xaxis is not None:
    x0 = x
    x = np.asarray(self.convert_xunits(x))
    width = self._convert_dx(width, x0, x, self.convert_xunits)
    ......

Similarly for height.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can generate plots with float x/y but Decimal width/height as this:
image

def test_barh_decimal_center(fig_test, fig_ref):
ax = fig_test.subplots()
Copy link
Member

Choose a reason for hiding this comment

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

Please move down as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have moved it down.

x0 = [1.5, 8.4, 5.3, 4.2]
y0 = [1.1, 2.2, 3.3, 4.4]
x = [Decimal(x) for x in x0]
y = [Decimal(y) for y in y0]
# Test image - horizontal, align-center bar chart with Decimal() input
ax.barh(x, y, height=[0.5, 0.5, 1, 1], align='center')
# Reference image
ax = fig_ref.subplots()
ax.barh(x0, y0, height=[0.5, 0.5, 1, 1], align='center')


@check_figures_equal(extensions=["png"])
def test_bar_decimal_width(fig_test, fig_ref):
x = [1.5, 8.4, 5.3, 4.2]
y = [1.1, 2.2, 3.3, 4.4]
w0 = [0.7, 1.45, 1, 2]
w = [Decimal(i) for i in w0]
# Test image - vertical bar chart with Decimal() width
ax = fig_test.subplots()
ax.bar(x, y, width=w, align='center')
# Reference image
ax = fig_ref.subplots()
ax.bar(x, y, width=w0, align='center')


@check_figures_equal(extensions=["png"])
def test_barh_decimal_height(fig_test, fig_ref):
x = [1.5, 8.4, 5.3, 4.2]
y = [1.1, 2.2, 3.3, 4.4]
h0 = [0.7, 1.45, 1, 2]
h = [Decimal(i) for i in h0]
# Test image - horizontal bar chart with Decimal() height
ax = fig_test.subplots()
ax.barh(x, y, height=h, align='center')
# Reference image
ax = fig_ref.subplots()
ax.barh(x, y, height=h0, align='center')


def test_bar_color_none_alpha():
ax = plt.gca()
rects = ax.bar([1, 2], [2, 4], alpha=0.3, color='none', edgecolor='r')
Expand Down Expand Up @@ -1819,6 +1876,21 @@ def test_scatter_2D(self):
fig, ax = plt.subplots()
ax.scatter(x, y, c=z, s=200, edgecolors='face')

@check_figures_equal(extensions=["png"])
def test_scatter_decimal(self, fig_test, fig_ref):
x0 = np.array([1.5, 8.4, 5.3, 4.2])
y0 = np.array([1.1, 2.2, 3.3, 4.4])
x = np.array([Decimal(i) for i in x0])
y = np.array([Decimal(i) for i in y0])
c = ['r', 'y', 'b', 'lime']
s = [24, 15, 19, 29]
# Test image - scatter plot with Decimal() input
ax = fig_test.subplots()
ax.scatter(x, y, c=c, s=s)
# Reference image
ax = fig_ref.subplots()
ax.scatter(x0, y0, c=c, s=s)

def test_scatter_color(self):
# Try to catch cases where 'c' kwarg should have been used.
with pytest.raises(ValueError):
Expand Down Expand Up @@ -5965,6 +6037,18 @@ def test_plot_columns_cycle_deprecation():
plt.plot(np.zeros((2, 2)), np.zeros((2, 3)))


@check_figures_equal(extensions=["png"])
def test_plot_decimal(fig_test, fig_ref):
x0 = np.arange(-10, 10, 0.3)
y0 = [5.2 * x ** 3 - 2.1 * x ** 2 + 7.34 * x + 4.5 for x in x0]
x = [Decimal(i) for i in x0]
y = [Decimal(i) for i in y0]
# Test image - line plot with Decimal input
fig_test.subplots().plot(x, y)
# Reference image
fig_ref.subplots().plot(x0, y0)


# pdf and svg tests fail using travis' old versions of gs and inkscape.
@check_figures_equal(extensions=["png"])
def test_markerfacecolor_none_alpha(fig_test, fig_ref):
Expand Down
55 changes: 55 additions & 0 deletions lib/matplotlib/units.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ def default_units(x, axis):
from numbers import Number

import numpy as np
from numpy import ma
from decimal import Decimal

from matplotlib import cbook

Expand Down Expand Up @@ -132,6 +134,58 @@ def is_numlike(x):
else:
return isinstance(x, Number)

@staticmethod
def is_natively_supported(x):
"""
Return whether *x* is of a type that Matplotlib natively supports or
*x* is array of objects of such types.
"""
# Matplotlib natively supports all number types except Decimal
if np.iterable(x):
# Assume lists are homogeneous as other functions in unit system
for thisx in x:
return (isinstance(thisx, Number) and
not isinstance(thisx, Decimal))
else:
return isinstance(x, Number) and not isinstance(x, Decimal)


class DecimalConverter(ConversionInterface):
"""
Converter for decimal.Decimal data to float.
"""
@staticmethod
def convert(value, unit, axis):
"""
Convert Decimals to floats.

The *unit* and *axis* arguments are not used.

Parameters
----------
value : decimal.Decimal or iterable
Decimal or list of Decimal need to be converted
"""
# If value is a Decimal
Copy link
Member

Choose a reason for hiding this comment

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

Can be left out. The following code already states exactly that.

if isinstance(value, Decimal):
return np.float(value)
else:
# assume x is a list of Decimal
converter = np.asarray
if isinstance(value, ma.MaskedArray):
converter = ma.asarray
return converter(value, dtype=np.float)

@staticmethod
def axisinfo(unit, axis):
# Since Decimal is a kind of Number, don't need specific axisinfo.
return AxisInfo()

@staticmethod
def default_units(x, axis):
# Return None since Decimal is a kind of Number.
return None


class Registry(dict):
"""Register types with conversion interface."""
Expand Down Expand Up @@ -164,3 +218,4 @@ def get_converter(self, x):


registry = Registry()
registry[Decimal] = DecimalConverter()