Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[REVIEW] Convert CSV reader CFFI to Cython #1453

Merged
merged 16 commits into from
Apr 22, 2019
Merged

[REVIEW] Convert CSV reader CFFI to Cython #1453

merged 16 commits into from
Apr 22, 2019

Conversation

kkraus14
Copy link
Collaborator

Related to #599 and #862

@kkraus14 kkraus14 added 2 - In Progress Currently a work in progress Python Affects Python cuDF API. Cython labels Apr 18, 2019
@kkraus14 kkraus14 changed the title [WIP] Convert CSV reader CFFI to Cython [REVIEW] Convert CSV reader CFFI to Cython Apr 22, 2019
@kkraus14 kkraus14 added 3 - Ready for Review Ready for review by team 4 - Needs cuDF Reviewer and removed 2 - In Progress Currently a work in progress labels Apr 22, 2019
@kkraus14 kkraus14 mentioned this pull request Apr 22, 2019
Merged
@kkraus14
Copy link
Collaborator Author

Some of this has been refactored in #1479 to deduplicate code and build better utility functions, but would rather have a single unit of work for review to start.

@kkraus14 kkraus14 added the cuIO cuIO issue label Apr 22, 2019
j-ieong
j-ieong reviewed Apr 22, 2019
View reviewed changes
Copy link
Contributor

@j-ieong j-ieong left a comment

Choose a reason for hiding this comment

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

Looks good to me overall. We'll probably change some stuff to more C++-like afterwards.

python/cudf/bindings/csv.pyx Show resolved Hide resolved
python/cudf/bindings/csv.pyx Outdated Show resolved Hide resolved
python/cudf/bindings/csv.pyx Outdated Show resolved Hide resolved
python/cudf/bindings/csv.pyx Show resolved Hide resolved
j-ieong and others added 2 commits April 22, 2019 15:37
@j-ieong @kkraus14
Free allocations from reader
6b840c1 
Co-Authored-By: kkraus14 <keith.j.kraus@gmail.com>
@j-ieong @kkraus14
Remove stale comment
83fbe08 
Co-Authored-By: kkraus14 <keith.j.kraus@gmail.com>
@kkraus14
Copy link
Collaborator Author

@j-ieong changes made to address all of your comments, thanks!

j-ieong
j-ieong approved these changes Apr 22, 2019
View reviewed changes
Copy link
Contributor

@j-ieong j-ieong left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@kkraus14 Looks great, thanks!

@harrism harrism merged commit e2bb320 into rapidsai:branch-0.7 Apr 22, 2019
@vyasr vyasr added 4 - Needs Review Waiting for reviewer to review or respond and removed 4 - Needs cuDF (Python) Reviewer labels Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakirkham jakirkham jakirkham left review comments

@j-ieong j-ieong j-ieong approved these changes

@harrism harrism harrism approved these changes

Assignees
No one assigned
Labels
3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond cuIO cuIO issue Python Affects Python cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kkraus14 @harrism @jakirkham @j-ieong @vyasr