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

User resource fix for replace_nulls #7769

Merged
merged 1 commit into from
Apr 1, 2021

Conversation

magnatelee
Copy link
Contributor

cudf::replace_nulls was copying the input column with the default stream and resource when there is no null. This simple PR is to make sure to pass the right stream and resource to the copy constructor.

@magnatelee magnatelee requested a review from a team as a code owner March 30, 2021 22:07
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Mar 30, 2021
@harrism harrism added bug Something isn't working non-breaking Non-breaking change labels Mar 30, 2021
Copy link
Contributor

@mythrocks mythrocks left a comment

Choose a reason for hiding this comment

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

LGTM!

@ttnghia
Copy link
Contributor

ttnghia commented Mar 31, 2021

Rerun tests.

@karthikeyann
Copy link
Contributor

rerun tests

@codecov
Copy link

codecov bot commented Mar 31, 2021

Codecov Report

Merging #7769 (6811c46) into branch-0.19 (7871e7a) will increase coverage by 0.78%.
The diff coverage is n/a.

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

@@               Coverage Diff               @@
##           branch-0.19    #7769      +/-   ##
===============================================
+ Coverage        81.86%   82.65%   +0.78%     
===============================================
  Files              101      103       +2     
  Lines            16884    17524     +640     
===============================================
+ Hits             13822    14484     +662     
+ Misses            3062     3040      -22     
Impacted Files Coverage Δ
python/cudf/cudf/utils/dtypes.py 83.27% <0.00%> (-6.24%) ⬇️
python/cudf/cudf/utils/gpu_utils.py 53.65% <0.00%> (-4.88%) ⬇️
python/cudf/cudf/core/column/lists.py 87.32% <0.00%> (-4.08%) ⬇️
python/dask_cudf/dask_cudf/backends.py 87.50% <0.00%> (-2.13%) ⬇️
python/cudf/cudf/core/abc.py 87.23% <0.00%> (-1.14%) ⬇️
python/cudf/cudf/core/column/decimal.py 93.84% <0.00%> (-1.03%) ⬇️
python/cudf/cudf/core/column/column.py 87.53% <0.00%> (-0.23%) ⬇️
python/cudf/cudf/utils/utils.py 85.36% <0.00%> (-0.07%) ⬇️
python/cudf/cudf/io/feather.py 100.00% <0.00%> (ø)
python/cudf/cudf/utils/ioutils.py 78.71% <0.00%> (ø)
... and 49 more

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 ad9212b...0bcea32. Read the comment docs.

@harrism
Copy link
Member

harrism commented Mar 31, 2021

@magnatelee can you look at the CI failures?

@kkraus14
Copy link
Collaborator

@magnatelee can you look at the CI failures?

error is unrelated. I think a CI hiccup that ops is actively dealing with.

@kkraus14
Copy link
Collaborator

rerun tests

@harrism
Copy link
Member

harrism commented Apr 1, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit e8f62ea into rapidsai:branch-0.19 Apr 1, 2021
rapids-bot bot pushed a commit that referenced this pull request Apr 12, 2021
Follow up on PR #7769
This PR fixes few more places of missing stream, and memory resource arguments.

Authors:
  - Karthikeyan (https://github.com/karthikeyann)

Approvers:
  - Ram (Ramakrishna Prabhu) (https://github.com/rgsl888prabhu)
  - Nghia Truong (https://github.com/ttnghia)

URL: #7779
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants