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

Fix comparisons between Series and cudf.NA #7072

Merged

Conversation

brandon-b-miller
Copy link
Contributor

Fixes #7043, gives less than ideal results due to #7066.

@brandon-b-miller brandon-b-miller added 2 - In Progress Currently a work in progress Python Affects Python cuDF API. non-breaking Non-breaking change labels Jan 4, 2021
@brandon-b-miller brandon-b-miller self-assigned this Jan 4, 2021
@brandon-b-miller brandon-b-miller requested a review from a team as a code owner January 4, 2021 23:50
@brandon-b-miller brandon-b-miller changed the title Fix cudfna comparisons Fix comparisons between Series and cudf.NA Jan 4, 2021
@brandon-b-miller brandon-b-miller added the bug Something isn't working label Jan 4, 2021
@codecov
Copy link

codecov bot commented Jan 5, 2021

Codecov Report

Merging #7072 (1550ba9) into branch-0.18 (8860baf) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.18    #7072      +/-   ##
===============================================
+ Coverage        82.09%   82.12%   +0.03%     
===============================================
  Files               97       97              
  Lines            16474    16498      +24     
===============================================
+ Hits             13524    13549      +25     
+ Misses            2950     2949       -1     
Impacted Files Coverage Δ
python/cudf/cudf/core/column/string.py 86.65% <100.00%> (ø)
python/cudf/cudf/core/series.py 91.10% <100.00%> (-0.06%) ⬇️
python/cudf/cudf/core/column/numerical.py 94.17% <0.00%> (-0.24%) ⬇️
python/cudf/cudf/core/column/lists.py 91.66% <0.00%> (-0.09%) ⬇️
python/cudf/cudf/core/reshape.py 91.00% <0.00%> (-0.04%) ⬇️
python/cudf/cudf/_fuzz_testing/fuzzer.py 0.00% <0.00%> (ø)
python/cudf/cudf/utils/hash_vocab_utils.py 100.00% <0.00%> (ø)
python/cudf/cudf/core/dataframe.py 90.76% <0.00%> (+0.05%) ⬆️
python/cudf/cudf/core/dtypes.py 90.50% <0.00%> (+0.12%) ⬆️
... and 3 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 8860baf...1550ba9. Read the comment docs.

@brandon-b-miller
Copy link
Contributor Author

rerun tests

@brandon-b-miller brandon-b-miller removed the 2 - In Progress Currently a work in progress label Jan 6, 2021
Copy link
Contributor

@galipremsagar galipremsagar left a comment

Choose a reason for hiding this comment

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

Should we add some unit-tests so that we can be sure we don't regress on this fix in the future while changing this part of code?

@brandon-b-miller
Copy link
Contributor Author

@galipremsagar yes we should add tests. When I sat down to write them though, the issue of #7066 came up. This PR causes the code to "work", but we get nan-like results. I am not sure if I should write tests that assert that logic since it's arguably not right.

Copy link
Contributor

@galipremsagar galipremsagar left a comment

Choose a reason for hiding this comment

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

Got it, probably we can just have an eq test case and xfail rest of the comparisons(lt, gt, ...) mentioning the #7066 in xfail reason.

@brandon-b-miller brandon-b-miller added the 3 - Ready for Review Ready for review by team label Jan 19, 2021
@galipremsagar galipremsagar added 5 - Ready to Merge Testing and reviews complete, ready to merge 6 - Okay to Auto-Merge and removed 3 - Ready for Review Ready for review by team labels Jan 19, 2021
@rapids-bot rapids-bot bot merged commit 8d80d5c into rapidsai:branch-0.18 Jan 19, 2021
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] Equality operation with cudf.NA fails with an error
2 participants