BUG: Handle iterable of arrays in convert #16026
Conversation
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 |
pandas/plotting/_converter.py
Outdated
| @@ -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]): | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
ahh could add that to the condition as well (so even more reason to make this an instrospection function).
There was a problem hiding this comment.
is_list_like(values) and len(values) and all(is_list_like(v) for v in values)
There was a problem hiding this comment.
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.
|
@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 |
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.
TomAugspurger
force-pushed
the
mpl-hist-2d
branch
from
5b9b29d to
e731cc0
Compare
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) |
There was a problem hiding this comment.
this one is not nested (needs a data = [data, data]?)
pandas/core/dtypes/inference.py
Outdated
|
|
||
| Examples | ||
| -------- | ||
| >>> is_list_like([1, 2, 3]) |
There was a problem hiding this comment.
These examples are still copied from the is_list_like docstring I think
| list, Series, np.array, tuple | ||
| ]) | ||
| def test_is_nested_list_like_passes(inner, outer): | ||
| result = outer([inner for _ in range(5)]) |
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