-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Changes from all commits
e1985d7
2279b16
19bb0c7
05dd388
61b491f
0a23d63
b7fdc9b
4be1d99
125dec7
eda2d01
109f252
cb82df8
7359ece
45da1c5
b2ca609
e293653
2c5a873
cb0a64c
b8ae7bd
124cc71
6e50219
8cbafae
b27afa3
ed456e4
7d13788
4005002
b290867
c56c175
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
import numpy as np | ||
from numpy import ma | ||
from cycler import cycler | ||
from decimal import Decimal | ||
import pytest | ||
|
||
import warnings | ||
|
@@ -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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just for symmetry, please move this down to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what happens if width (or height) is a Decimal? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Similarly for height. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
def test_barh_decimal_center(fig_test, fig_ref): | ||
ax = fig_test.subplots() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please move down as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') | ||
|
@@ -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): | ||
|
@@ -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): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.""" | ||
|
@@ -164,3 +218,4 @@ def get_converter(self, x): | |
|
||
|
||
registry = Registry() | ||
registry[Decimal] = DecimalConverter() |
There was a problem hiding this comment.
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 beThat will allow to cleanly pass through
Decimal
s.There was a problem hiding this comment.
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 thannot needs_conversion(x)
.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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.There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
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?
There was a problem hiding this comment.
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 useConversionInterface.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 asis_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 theis_natively_supported*
docstring.is_numlike(x)
may become deprecated in the future, but we can handle this later.There was a problem hiding this comment.
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.