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

Logical operator style fix in reduce.cuh #10670

Closed

Conversation

isVoid
Copy link
Contributor

@isVoid isVoid commented Apr 15, 2022

Addressing #10447 (comment), logical operators should be consistent in a statement.

@isVoid isVoid requested a review from a team as a code owner April 15, 2022 18:25
@isVoid isVoid requested review from trxcllnt and nvdbaranec April 15, 2022 18:25
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Apr 15, 2022
@isVoid isVoid added 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change libcudf Affects libcudf (C++/CUDA) code. and removed libcudf Affects libcudf (C++/CUDA) code. labels Apr 15, 2022
@jrhemstad
Copy link
Contributor

I don't have any qualms with mixing && and not. I almost always use not instead of ! because ! is too easy to overlook when reading a statement.

@codecov
Copy link

codecov bot commented Apr 15, 2022

Codecov Report

Merging #10670 (cadd38b) into branch-22.06 (3c13ef1) will increase coverage by 0.03%.
The diff coverage is 90.51%.

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

@@               Coverage Diff                @@
##           branch-22.06   #10670      +/-   ##
================================================
+ Coverage         86.33%   86.37%   +0.03%     
================================================
  Files               140      142       +2     
  Lines             22289    22356      +67     
================================================
+ Hits              19244    19309      +65     
- Misses             3045     3047       +2     
Impacted Files Coverage Δ
python/cudf/cudf/core/_base_index.py 85.45% <ø> (ø)
python/cudf/cudf/core/algorithms.py 90.47% <ø> (ø)
python/cudf/cudf/core/cut.py 82.69% <ø> (ø)
python/cudf/cudf/core/groupby/groupby.py 91.72% <ø> (+0.22%) ⬆️
python/cudf/cudf/core/indexed_frame.py 91.77% <ø> (ø)
python/cudf/cudf/core/multiindex.py 92.14% <ø> (ø)
python/cudf/cudf/core/series.py 95.28% <ø> (ø)
python/cudf/cudf/core/tools/numeric.py 89.24% <ø> (ø)
python/cudf/cudf/utils/cudautils.py 59.83% <ø> (ø)
python/cudf/cudf/utils/ioutils.py 79.60% <ø> (ø)
... and 16 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 f7c35d5...f2c91d1. Read the comment docs.

@bdice
Copy link
Contributor

bdice commented Apr 18, 2022

I feel similarly to @jrhemstad on this — I would rather change && to and than not to ! if we had a consensus that we like consistency in logical operator forms. The full words also make the statement easier to read (à la Python). However I don’t think we need to do anything here and I’d vote to close this.

@isVoid
Copy link
Contributor Author

isVoid commented Apr 18, 2022

That's ok with me. It irritates me a little bit when I see inconsistencies in the same file with ! and not such as here: https://github.com/rapidsai/cudf/pull/10447/files#r851226348. But since we are still don't have a consensus to this I'm ok to close this.

@isVoid isVoid closed this Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants