Skip to content

Refactoring propositions #70

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

Closed
wants to merge 3 commits into from
Closed

Conversation

iskode
Copy link
Contributor

@iskode iskode commented Oct 29, 2021

I've made a few changes based on the feedback from the cudf implementation PR I've submit still under review.
There 3 main refactoring (one for each commit):

  1. The first part of the buffer_to_ndarray method is convert the protocol dtype to numpy one. I've refactored it in a dedicated method as in cudf implementation, I needed it a dozen of times so this might be the case for pandas in near future. Also this method used some local constants that I've moved into module level constant. This will prevent to recreate these variables for each call.
  2. I've moved the Device class from the__dlpack_device__ method to the module level as _DtypeKind. Then I've add all the devices values so that the code will show which are currently support and maintainers will know directly about future device supports.
  3. I've noticed that convert_string_column return all buffers while buffer_to_ndarray and convert_categorical_column return only the data buffer. So I've set these latter to return all buffers as well.

@rgommers
Copy link
Member

Thanks a lot @iskode. Given that after gh-81 the implementation has been removed from this repo, and Pandas now has its own implementation merged (with significant improvements compared to the prototype that this PR was refactoring), I will close this PR.

We did learn quite a bit from your cuDF implementation work. Thanks for opening this PR, and apologies it did not get merged last year.

@rgommers rgommers closed this Jul 28, 2022
@iskode
Copy link
Contributor Author

iskode commented Jul 29, 2022

No problem. You don't have to apologies !
Thank you for your mentoring. It has been very great experience.

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

Successfully merging this pull request may close these issues.

2 participants