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

Replace cudf.logical_not with ~ #4669

Merged
merged 4 commits into from
Mar 31, 2022
Merged

Replace cudf.logical_not with ~ #4669

merged 4 commits into from
Mar 31, 2022

Conversation

canonizer
Copy link
Contributor

@canonizer canonizer commented Mar 30, 2022 •
edited

Replace cudf.logical_not with ~.

@canonizer canonizer requested a review from a team as a code owner March 30, 2022 14:15
@github-actions github-actions bot added the Cython / Python Cython or Python issue label Mar 30, 2022
@canonizer
Copy link
Contributor Author

rerun tests

bdice
bdice reviewed Mar 30, 2022
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

A couple suggestions for conciseness. I recently refactored part of cuDF to remove the use of cudf.logical_not and used these patterns so I thought they might be worth suggesting.

python/cuml/preprocessing/text/stem/porter_stemmer.py Outdated Show resolved Hide resolved
python/cuml/preprocessing/text/stem/porter_stemmer.py Outdated Show resolved Hide resolved
python/cuml/preprocessing/text/stem/porter_stemmer_utils/porter_stemmer_rules.py Outdated Show resolved Hide resolved
@canonizer canonizer changed the title Replace cudf.logical_not with numpy.logical_not Replace cudf.logical_not with ~ Mar 30, 2022
@canonizer
Copy link
Contributor Author

@bdice @beckernick Thanks for your comments! I've addressed them, could you take another look?

bdice
bdice approved these changes Mar 30, 2022
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment •
edited

Choose a reason for hiding this comment

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

Looks good to me. I'm not on the cuML codeowners / reviewer list but this should fix the problem with the cudf.logical_not API removed in cuDF 22.06.

@dantegd dantegd added bug Something isn't working non-breaking Non-breaking change labels Mar 30, 2022
@dantegd
Copy link
Member

dantegd commented Mar 31, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit bfe034a into rapidsai:branch-22.06 Mar 31, 2022
venkywonka pushed a commit to venkywonka/cuml that referenced this pull request Apr 26, 2022
@canonizer @venkywonka
Replace cudf.logical_not with ~ (rapidsai#4669)
aac34a4 
Replace `cudf.logical_not` with `~`.

Authors:
  - Andy Adinets (https://github.com/canonizer)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: rapidsai#4669
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
@canonizer
Replace cudf.logical_not with ~ (rapidsai#4669)
9916c29 
Replace `cudf.logical_not` with `~`.

Authors:
  - Andy Adinets (https://github.com/canonizer)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Dante Gama Dessavre (https://github.com/dantegd)

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

@beckernick beckernick beckernick left review comments

@dantegd dantegd dantegd approved these changes

@bdice bdice bdice approved these changes

Assignees
No one assigned
Labels
bug Something isn't working Cython / Python Cython or Python issue non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@canonizer @dantegd @bdice @beckernick