BUG: Handle iterable of arrays in convert by TomAugspurger · Pull Request #16026 · pandas-dev/pandas (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation14 Commits3 Checks0 Files changed

Conversation

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]({{ revealButtonHref }})

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,

Presumably our other two converters TimeConverter and PeriodConverter could suffer from the same problem?
if is_list_like(values) and is_list_like(values[0]): feels hacky... Thoughts?

xref matplotlib/matplotlib#8459

I would also suspect.
we have similar looking stuff to deal with ambiguous import from the user. That looks reasonable to me. Alternative is to try...except passing to the 1d version and then fall back to the list comprehension?

Nice solution. One slight concern though: line 184 of _converter.py looks a bit buggy:
if is_list_like(values) and is_list_like(values[0]):
If values == [], it looks like is_list_like(values[0]) will cause an error while is_list_like(values) still returns True

@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]):

Choose a reason for hiding this comment

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

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)

Choose a reason for hiding this comment

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

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)

Choose a reason for hiding this comment

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

ahh could add that to the condition as well (so even more reason to make this an instrospection function).

Choose a reason for hiding this comment

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

is_list_like(values) and len(values) and all(is_list_like(v) for v in values)

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

whatsnew note, comment on impl. otherwise lgtm.

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.

Can you also add a small test for the Converter itself?

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)

Choose a reason for hiding this comment

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

this one is not nested (needs a data = [data, data]?)


Examples
--------
>>> is_list_like([1, 2, 3])

Choose a reason for hiding this comment

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

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)])

Choose a reason for hiding this comment

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

nice!

BUG: Handle iterable of arrays in convert by TomAugspurger · Pull Request #16026 · pandas-dev/pandas (original) (raw)

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels