Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
BUG: Handle iterable of arrays in convert #16026
Conversation
TomAugspurger
added the
Visualization
label
Apr 17, 2017
TomAugspurger
referenced
this pull request
in matplotlib/matplotlib
Apr 17, 2017
Closed
plt.hist: Unable to plot multiple distributions when x is a datetime #8459
codecov
bot
commented
Apr 17, 2017
•
Codecov Report
@@ Coverage Diff @@
## master #16026 +/- ##
==========================================
+ Coverage 90.99% 91% +<.01%
==========================================
Files 153 153
Lines 50469 50481 +12
==========================================
+ Hits 45924 45939 +15
+ Misses 4545 4542 -3
Continue to review full report at Codecov.
|
|
|
Nice solution. One slight concern though: line 184 of |
| @@ -178,6 +180,16 @@ class DatetimeConverter(dates.DateConverter): | ||
| @staticmethod | ||
| def convert(values, unit, axis): | ||
| + # values might be a 1-d array, or a list-like of arrays. | ||
| + if is_list_like(values) and is_list_like(values[0]): |
jreback
Apr 17, 2017
Contributor
use
is_list_like(values) and all(is_list_like(v) for v in values).
you could make this a method in pandas.core.dtypes.common if you want (with tests!) maybe
is_list_like_nested (I don't know if there is a case where any of the sub-lists is True but you don't want all)
jorisvandenbossche
Apr 17, 2017
Owner
A (minor) problem with all(is_list_like(v) for v in values) is that this returns True for an empty list (the values=[] case).
It's only minor in that it still works but the final return result will become an empty list instead of an empty float array. Which might be a problem.
(I am not sure to what extent a Converter is supposed to return an array in matplotlib code)
jreback
Apr 17, 2017
Contributor
ahh could add that to the condition as well (so even more reason to make this an instrospection function).
jreback
Apr 17, 2017
Contributor
is_list_like(values) and len(values) and all(is_list_like(v) for v in values)
TomAugspurger
Apr 17, 2017
Contributor
That's roughly what I ended up doing. One issue is that I don't want this accidentally consuming a generator, so if the outer container doesn't define len we return False. Will push in a few minutes.
jreback
added the
Bug
label
Apr 17, 2017
|
@TomAugspurger Can you also add a small test for the Converter itself? https://github.com/pandas-dev/pandas/blob/master/pandas/tests/plotting/test_converter.py#L30 eg just an example with that string in a nested list |
Added a test for PeriodConverter and DateTimeConverter. I don't see tests for TimeConverter, so I haven't touched that code at all. |
| + | ||
| + def test_convert_nested(self): | ||
| + data = ['2012-1-1', '2012-1-2'] | ||
| + r1 = self.pc.convert(data, None, self.axis) |
| + | ||
| + Examples | ||
| + -------- | ||
| + >>> is_list_like([1, 2, 3]) |
jorisvandenbossche
Apr 17, 2017
Owner
These examples are still copied from the is_list_like docstring I think
|
|
TomAugspurger |
All checks have passed |
a786627
|
| + list, Series, np.array, tuple | ||
| +]) | ||
| +def test_is_nested_list_like_passes(inner, outer): | ||
| + result = outer([inner for _ in range(5)]) |
TomAugspurger commentedApr 17, 2017
DatetimeConverter.convert can take an array or iterable of arrays.
Fixed the converter to detect which case we're in and then re-use
the existing logic.
Posting here for feedback from @tacaswell. Specifically,
TimeConverterandPeriodConvertercould suffer from the same problem?if is_list_like(values) and is_list_like(values[0]):feels hacky... Thoughts?xref matplotlib/matplotlib#8459