only interchange necessary columns #4286
Conversation
|
🤔 bit confused by the CI failure, it fails on "install chrome driver"? |
|
@LiamConnors would you mind making another "pin chrome" PR in this repo? (Not in this PR but so this and other PRs can update and succeed again!) |
|
Wow, I'm excited that someone is biting the bullet on this one, thank you! Would be nice for this to be reused for the jankier |
Opened a PR here to fix it: #4288 @alexcjohnson |
Sure, but there's no guarantee of what the API to do that would be, right? Before having called |
| df_pandas = df_not_pandas.to_pandas() | ||
| args["data_frame"] = df_pandas | ||
| args["data_frame"] = df_not_pandas.__dataframe__() | ||
| columns = args["data_frame"].column_names() |
There was a problem hiding this comment.
Can we be 100% sure anything returned by __dataframe__() will have column_names and select_columns_by_name methods? If there's any chance an object will come in with either of these missing we should fall back on interchanging the whole thing up front.
There was a problem hiding this comment.
yup! you can check the spec here: https://data-apis.org/dataframe-protocol/latest/API.html
There was a problem hiding this comment.
Oh I know they're in the spec, but I also know not everyone follows a spec to the letter 😉
There was a problem hiding this comment.
this can help highlight shortcomings in their implementation then 😉 I tried it out with polars and it works fine there
There was a problem hiding this comment.
I guess we'll know how to respond when we see:
AttributeError: 'MyDataFrame' object has no attribute 'select_columns_by_name'
And in principle you're right that it's not our problem, but we'll be the ones responding to the issue and having to tell our users "don't use this dataframe directly until they fix it." Whereas if we caught this case explicitly we could emit a warning like "This dataframe only partially implements the dataframe interchange protocol. Falling back on a slower full-copy algorithm" so it wouldn't affect usage in px, only performance, and it would be clear where the issue needs to be raised.
There was a problem hiding this comment.
thanks for explaining - OK I've added a condition so it'll only use select_columns_by_name if that attribute is present
There was a problem hiding this comment.
Nice. I took a look at also adding a fallback for missing column_names and that would be pretty awkward... but if someone has a partial implementation of the protocol presumably column_names is an easy piece so would get included early, whereas select_columns_by_name could be trickier. So let's leave it as you have it now. Thanks!
There was a problem hiding this comment.
yeah if they don't have column_names then from_dataframe wouldn't work either, as it uses that internally
Heh, no, I think it's me that's forgotten that this is exactly why we have the data-interchange protocol, you're right ;) |
Trying to address this comment: #3901 (comment)
If it's not a wide plot, then only interchange the columns which are needed