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

Support constructing cudf.Scalar objects from host side lists #8459

Merged
merged 18 commits into from
Jun 25, 2021
Merged

Support constructing cudf.Scalar objects from host side lists #8459

merged 18 commits into from
Jun 25, 2021

Conversation

brandon-b-miller
Copy link
Contributor

Makes it so that this works:

>>> slr = cudf.Scalar([1,2,3])
>>> slr
Scalar([1, 2, 3], dtype=list)
>>> slr.dtype
ListDtype(int64)

Required for ListColumn.__setitem__

@brandon-b-miller brandon-b-miller requested a review from a team as a code owner June 8, 2021 15:28
@github-actions github-actions bot added the Python Affects Python cuDF API. label Jun 8, 2021
@brandon-b-miller brandon-b-miller added 2 - In Progress Currently a work in progress Cython feature request New feature or request non-breaking Non-breaking change labels Jun 8, 2021
@brandon-b-miller brandon-b-miller added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Jun 11, 2021
@codecov
Copy link

codecov bot commented Jun 11, 2021 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.08@671b76d). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head d742ebb differs from pull request most recent head cff4b79. Consider uploading reports for the commit cff4b79 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.08    #8459   +/-   ##
===============================================
  Coverage                ?   83.00%           
===============================================
  Files                   ?      109           
  Lines                   ?    18231           
  Branches                ?        0           
===============================================
  Hits                    ?    15132           
  Misses                  ?     3099           
  Partials                ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 671b76d...cff4b79. Read the comment docs.

@galipremsagar
Copy link
Contributor

rerun tests

@galipremsagar galipremsagar added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Jun 14, 2021
isVoid
isVoid approved these changes Jun 14, 2021
View reviewed changes
Copy link
Contributor

@isVoid isVoid left a comment

Choose a reason for hiding this comment

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

Would love to revisit:

cudf/python/cudf/cudf/tests/test_groupby.py

Lines 1788 to 1789 in d30695c

# TODO: Shifting list columns is currently unsupported because we cannot
# construct a null list scalar in python. Support once it is added.

after this is merged. (Can't remember what's the exact issue)

@brandon-b-miller
Copy link
Contributor Author

Would love to revisit:

cudf/python/cudf/cudf/tests/test_groupby.py

Lines 1788 to 1789 in d30695c

# TODO: Shifting list columns is currently unsupported because we cannot
# construct a null list scalar in python. Support once it is added.

after this is merged. (Can't remember what's the exact issue)

I think it has to do with null filling, it probably fails to construct here using a listdtype:

cudf/python/cudf/cudf/_lib/copying.pyx

Line 660 in d30695c

fill = as_device_scalar(fill_value, input.dtype)

As an aside, thank you for pointing that out as this case is actually totally unhandled in this PR - I think it makes sense to go back and add it here before merging.

@brandon-b-miller
Copy link
Contributor Author

@isVoid could you request changes on this PR to temporarily block merging?

@brandon-b-miller brandon-b-miller added 2 - In Progress Currently a work in progress and removed 5 - Ready to Merge Testing and reviews complete, ready to merge labels Jun 14, 2021
isVoid
isVoid requested changes Jun 14, 2021
View reviewed changes
Copy link
Contributor

@isVoid isVoid left a comment

Choose a reason for hiding this comment

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

Request change per above.

@brandon-b-miller brandon-b-miller added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Jun 17, 2021
@brandon-b-miller
Copy link
Contributor Author

This should be ready for another look.

isVoid
isVoid approved these changes Jun 17, 2021
View reviewed changes
Copy link
Contributor

@isVoid isVoid left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for addressing above.

python/cudf/cudf/core/column/column.py Outdated Show resolved Hide resolved
@beckernick beckernick mentioned this pull request Jun 18, 2021
Closed
2 tasks
@brandon-b-miller
Copy link
Contributor Author

Discovered a bug while working forward on __setitem__ should be resolved by the latest commit.

@brandon-b-miller
Copy link
Contributor Author

rerun tests

@brandon-b-miller brandon-b-miller mentioned this pull request Jun 24, 2021
Merged
@brandon-b-miller
Copy link
Contributor Author

rerun tests

@brandon-b-miller brandon-b-miller added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Jun 25, 2021
@brandon-b-miller
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 58a31c1 into rapidsai:branch-21.08 Jun 25, 2021
rapids-bot bot pushed a commit that referenced this pull request Jun 30, 2021
@brandon-b-miller
ListColumn __setitem__ (#8606)
5884b95 
Closes #8553
based on #8459

Authors:
  - https://github.com/brandon-b-miller

Approvers:
  - Ashwin Srinath (https://github.com/shwina)
  - GALI PREM SAGAR (https://github.com/galipremsagar)

URL: #8606
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marlenezw marlenezw marlenezw approved these changes

@isVoid isVoid isVoid approved these changes

@galipremsagar galipremsagar galipremsagar approved these changes

@skirui-source skirui-source Awaiting requested review from skirui-source

Assignees
No one assigned
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge feature request New feature or request non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@brandon-b-miller @galipremsagar @isVoid @marlenezw