-
-
Notifications
You must be signed in to change notification settings - Fork 125
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
ENH: add from_dataframe and DataFrame #331
Conversation
bashtage
commented
Sep 26, 2022
- xref Version 1.5 changes #227
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.
Unclear to me whether we should add tests for this? Other comments are related to checking the API definition at https://data-apis.org/dataframe-protocol/latest/API.html, which may reveal that we have issues in the pandas source in how this is typed.
@abstractmethod | ||
def __dlpack__(self): ... | ||
@abstractmethod | ||
def __dlpack_device__(self) -> tuple[DlpackDeviceType, int | None]: ... |
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.
Looking at https://data-apis.org/dataframe-protocol/latest/API.html, I don't think the second element of the tuple can be None
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.
Here
it is None
. This is a concrete implementation.
class Column(ABC, metaclass=abc.ABCMeta): | ||
@property | ||
@abstractmethod | ||
def size(self) -> int: ... |
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.
Looking at https://data-apis.org/dataframe-protocol/latest/API.html, returns Optional[int]
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 the spec type might also be wrong here. The text indicates that the size is required, either the full DF size or the chunk size if a single chunk.
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.
thanks @bashtage