Skip to content

Add allow_copy flag to interchange protocol#51

Merged
rgommers merged 5 commits intodata-apis:mainfrom
rgommers:allow-copy
Aug 24, 2021
Merged

Add allow_copy flag to interchange protocol#51
rgommers merged 5 commits intodata-apis:mainfrom
rgommers:allow-copy

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.

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