Skip to content

Add allow_copy flag to interchange protocol #51

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

Merged
merged 5 commits into from
Aug 24, 2021

Conversation

rgommers
Copy link
Member

Rebase and fix up of gh-44.

steff456 and others added 4 commits August 23, 2021 18:11
This PR adds a flag to throw an exception if the export cannot be
zero-copy. (e.g. for pandas, possible due to block manager where rows
are contiguous and columns are not) .

- Add `allow_zero_copy` flag to the DataFrame class.
- Propagate the flag to the buffer and raise a `RuntimeError` when it is true
- Fix `test_noncontiguous_columns`
- Make update in the requirements doc
Copy link
Contributor

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

LGTM. Nothing obvious jumps out at me. One minor clarification question left in the review regarding when we're choosing to pass allow_copy.

dtype = self.dtype
elif self.dtype[0] == _k.CATEGORICAL:
codes = self._col.values.codes
buffer = _PandasBuffer(codes)
buffer = _PandasBuffer(
Copy link
Contributor

Choose a reason for hiding this comment

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

As a sanity check, we're not similarly passing allow_copy at L595, L634, and L676 because we have guaranteed contiguous buffers in those cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes indeed. np.asarray(some_list) creates a new contiguous array, and the bytearray usage seems to be contiguous too (that one was a bit harder to verify, but I did test it).

@rgommers rgommers merged commit bcb5024 into data-apis:main Aug 24, 2021
@rgommers rgommers deleted the allow-copy branch August 24, 2021 11:38
@rgommers
Copy link
Member Author

Okay, in it goes then. Thanks for verifying @kgryte

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants