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.

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

[REVIEW] Support cupy array in quantile input #10429

Merged
merged 16 commits into from
Mar 21, 2022

Conversation

galipremsagar
Copy link
Contributor

Fixes: #8288 This PR fixes Series.quantile input q to support cupy array's.

@galipremsagar galipremsagar added bug Something isn't working 3 - Ready for Review Ready for review by team Python Affects Python cuDF API. 4 - Needs cuDF (Python) Reviewer non-breaking Non-breaking change labels Mar 14, 2022
@galipremsagar galipremsagar requested a review from vyasr March 14, 2022 17:44
@galipremsagar galipremsagar requested a review from a team as a code owner March 14, 2022 17:44
@galipremsagar galipremsagar self-assigned this Mar 14, 2022
@codecov
Copy link

codecov bot commented Mar 14, 2022

Codecov Report

Merging #10429 (67c4418) into branch-22.04 (21ed251) will increase coverage by 0.18%.
The diff coverage is 100.00%.

❗ Current head 67c4418 differs from pull request most recent head 7b28368. Consider uploading reports for the commit 7b28368 to get more accurate results

@@               Coverage Diff                @@
##           branch-22.04   #10429      +/-   ##
================================================
+ Coverage         85.95%   86.14%   +0.18%     
================================================
  Files               139      139              
  Lines             22435    22435              
================================================
+ Hits              19285    19326      +41     
+ Misses             3150     3109      -41     
Impacted Files Coverage Δ
python/cudf/cudf/core/column/column.py 89.27% <ø> (ø)
python/cudf/cudf/core/column/datetime.py 88.51% <100.00%> (-0.05%) ⬇️
python/cudf/cudf/core/column/numerical_base.py 98.88% <100.00%> (+1.06%) ⬆️
python/cudf/cudf/core/column/timedelta.py 90.45% <100.00%> (-0.05%) ⬇️
python/cudf/cudf/core/series.py 95.18% <100.00%> (+0.01%) ⬆️
python/cudf/cudf/core/column/string.py 88.91% <0.00%> (+0.12%) ⬆️
python/cudf/cudf/core/groupby/groupby.py 91.72% <0.00%> (+0.22%) ⬆️
python/cudf/cudf/core/tools/datetimes.py 84.49% <0.00%> (+0.30%) ⬆️
python/cudf/cudf/core/column/lists.py 90.69% <0.00%> (+0.46%) ⬆️
...thon/dask_cudf/dask_cudf/tests/test_distributed.py 86.79% <0.00%> (+67.92%) ⬆️

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 40baeb4...7b28368. Read the comment docs.

@galipremsagar galipremsagar requested a review from vyasr March 15, 2022 17:39
python/cudf/cudf/tests/test_stats.py Show resolved Hide resolved
python/cudf/cudf/core/series.py Outdated Show resolved Hide resolved
@galipremsagar galipremsagar requested a review from vyasr March 18, 2022 23:10
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

In addition to my one inline comment, I have one last question. Does it makes sense for the return_scalar logic to exist in the column layer at all, or would it be simpler to remove that parameter and always unpack the scalar in the Series method? In the interest of getting this merged for the release I'd be fine with postponing that change if you don't think it's quick to assess that now, but I think it's worth considering.

index = None

return Series(result, index=index, name=self.name)
return Series(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this use _from_data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah updated to use _from_data

@galipremsagar
Copy link
Contributor Author

In addition to my one inline comment, I have one last question. Does it makes sense for the return_scalar logic to exist in the column layer at all, or would it be simpler to remove that parameter and always unpack the scalar in the Series method? In the interest of getting this merged for the release I'd be fine with postponing that change if you don't think it's quick to assess that now, but I think it's worth considering.

I have given it a thought before but realized it'll end-up duplicating a lot of code for other methods like median which call in quantile, hence deferred to make those changes for now.

@vyasr
Copy link
Contributor

vyasr commented Mar 21, 2022

In addition to my one inline comment, I have one last question. Does it makes sense for the return_scalar logic to exist in the column layer at all, or would it be simpler to remove that parameter and always unpack the scalar in the Series method? In the interest of getting this merged for the release I'd be fine with postponing that change if you don't think it's quick to assess that now, but I think it's worth considering.

I have given it a thought before but realized it'll end-up duplicating a lot of code for other methods like median which call in quantile, hence deferred to make those changes for now.

Yeah I noticed the same. Fine with me to defer, good to know you're already thinking about it 👍

python/cudf/cudf/core/series.py Outdated Show resolved Hide resolved
@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 4 - Needs cuDF (Python) Reviewer labels Mar 21, 2022
@galipremsagar
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 0ffd718 into rapidsai:branch-22.04 Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge bug Something isn't working non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] bug passing in cp.array in quantile function
2 participants