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

Removed mr parameter from inplace bitmask operations #10805

Merged
merged 8 commits into from
May 25, 2022
Merged

Removed mr parameter from inplace bitmask operations #10805

merged 8 commits into from
May 25, 2022

Conversation

AtlantaPepsi
Copy link
Contributor

@AtlantaPepsi AtlantaPepsi commented May 6, 2022 •
edited by PointKernel
Loading

Closes #10763

This PR removed mr parameter from inplace bitmask operations by following guidance for temporary memory allocation.

@GPUtester
Copy link
Collaborator

Can one of the admins verify this patch?

@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label May 6, 2022
@AtlantaPepsi AtlantaPepsi marked this pull request as ready for review May 6, 2022 03:07
@AtlantaPepsi AtlantaPepsi requested a review from a team as a code owner May 6, 2022 03:07
@AtlantaPepsi AtlantaPepsi marked this pull request as draft May 6, 2022 03:08
@karthikeyann karthikeyann added 2 - In Progress Currently a work in progress 4 - Needs Review Waiting for reviewer to review or respond tech debt improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels May 6, 2022
@karthikeyann
Copy link
Contributor

rerun tests

@AtlantaPepsi
Copy link
Contributor Author

AtlantaPepsi commented May 12, 2022 •
edited
Loading

rerun tests

@karthikeyann
yeah sorry my cluster is in the middle maintenance now, and I don't have device to run tests until later

@sevagh
Copy link
Contributor

sevagh commented May 12, 2022

ok to test

@PointKernel
Copy link
Member

PointKernel commented May 12, 2022 •
edited
Loading

@AtlantaPepsi Thanks for the contribution!

The CI style check should pass once you format the code by following the instructions here. As you removed the mr argument in inplace_bitmask_binop, can you please also update the corresponding doc (by removing this line)?

@AtlantaPepsi AtlantaPepsi marked this pull request as ready for review May 14, 2022 20:01
@AtlantaPepsi AtlantaPepsi marked this pull request as draft May 14, 2022 21:15
@AtlantaPepsi AtlantaPepsi marked this pull request as ready for review May 15, 2022 02:14
@codecov
Copy link

codecov bot commented May 15, 2022 •
edited
Loading

Codecov Report

Merging #10805 (cd5caee) into branch-22.06 (8d861ce) will decrease coverage by 0.07%.
The diff coverage is 92.92%.

❗ Current head cd5caee differs from pull request most recent head be77d99. Consider uploading reports for the commit be77d99 to get more accurate results

@@               Coverage Diff                @@
##           branch-22.06   #10805      +/-   ##
================================================
- Coverage         86.40%   86.32%   -0.08%     
================================================
  Files               143      144       +1     
  Lines             22448    22665     +217     
================================================
+ Hits              19396    19566     +170     
- Misses             3052     3099      +47
Impacted Files Coverage Δ
python/cudf/cudf/core/_base_index.py 85.71% <ø> (ø)
python/cudf/cudf/core/column/timedelta.py 90.75% <ø> (ø)
python/cudf/cudf/core/cut.py 82.69% <ø> (ø)
python/cudf/cudf/core/frame.py 93.41% <ø> (ø)
python/cudf/cudf/core/groupby/groupby.py 91.79% <ø> (+0.22%) ⬆️
python/cudf/cudf/core/indexed_frame.py 91.70% <ø> (ø)
python/cudf/cudf/core/join/join.py 94.19% <ø> (ø)
python/cudf/cudf/core/multiindex.py 92.28% <ø> (ø)
python/cudf/cudf/core/resample.py 88.97% <ø> (ø)
python/cudf/cudf/core/subword_tokenizer.py 75.00% <ø> (ø)
... and 43 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 4ce7b65...be77d99. Read the comment docs.

@AtlantaPepsi @PointKernel
fixing copyright year
385b169 
Co-authored-by: Yunsong Wang <wangyunsong89@gmail.com>
@PointKernel PointKernel added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels May 17, 2022
@PointKernel @karthikeyann
Update cpp/include/cudf/detail/null_mask.hpp
be77d99 
Co-authored-by: Karthikeyan <6488848+karthikeyann@users.noreply.github.com>
@karthikeyann karthikeyann self-requested a review May 22, 2022 18:09
@PointKernel PointKernel changed the title removed mr parameter (temporary) from inplace bitmask operations Removed mr parameter from inplace bitmask operations May 25, 2022
@PointKernel
Copy link
Member

@gpucibot merge

@rapids-bot rapids-bot bot merged commit df5dc08 into rapidsai:branch-22.06 May 25, 2022
@codereport codereport removed their request for review May 25, 2022 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karthikeyann karthikeyann karthikeyann approved these changes

@PointKernel PointKernel PointKernel approved these changes

@cwharris cwharris Awaiting requested review from cwharris cwharris is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

Assignees
No one assigned
Labels
3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove mr parameter from inplace bitmask operations.
5 participants
@AtlantaPepsi @GPUtester @karthikeyann @sevagh @PointKernel