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

Improve assert_eq handling of scalar #7220

Merged
merged 3 commits into from
Feb 4, 2021
Merged

Conversation

isVoid
Copy link
Contributor

@isVoid isVoid commented Jan 26, 2021

Closes #7199

Refactors scalar handling inside assert_eq. On higher level, this PR proposes a "whitelist" style testing: all compares should go to the "strict equal" code path unless explicitly allowed. This allows the test system to capture all unintended inequality except the ones that's discussed upon. For example, this PR creates two whitelist items:

  • If the operands overrides __eq__, use it to determine equality.
  • If the operands are floating type, assert approximate equality.
    For all other cases, the operands should be strictly equal. Note that for testing purposes, np.nan are considered equal to itself.

@isVoid isVoid requested a review from a team as a code owner January 26, 2021 21:38
@isVoid isVoid added 3 - Ready for Review Ready for review by team bug Something isn't working Python Affects Python cuDF API. non-breaking Non-breaking change tests Unit testing for project labels Jan 26, 2021
@isVoid isVoid self-assigned this Jan 26, 2021
@codecov
Copy link

codecov bot commented Jan 27, 2021

Codecov Report

Merging #7220 (4198c48) into branch-0.18 (8860baf) will increase coverage by 0.11%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.18    #7220      +/-   ##
===============================================
+ Coverage        82.09%   82.20%   +0.11%     
===============================================
  Files               97      100       +3     
  Lines            16474    16952     +478     
===============================================
+ Hits             13524    13936     +412     
- Misses            2950     3016      +66     
Impacted Files Coverage Δ
python/cudf/cudf/__init__.py 100.00% <ø> (ø)
python/cudf/cudf/_fuzz_testing/avro.py 0.00% <ø> (ø)
python/cudf/cudf/_fuzz_testing/csv.py 0.00% <ø> (ø)
python/cudf/cudf/_fuzz_testing/fuzzer.py 0.00% <ø> (ø)
python/cudf/cudf/_fuzz_testing/io.py 0.00% <ø> (ø)
python/cudf/cudf/_fuzz_testing/json.py 0.00% <ø> (ø)
python/cudf/cudf/_fuzz_testing/main.py 0.00% <ø> (ø)
python/cudf/cudf/_fuzz_testing/orc.py 0.00% <ø> (ø)
python/cudf/cudf/_fuzz_testing/parquet.py 0.00% <ø> (ø)
python/cudf/cudf/_fuzz_testing/utils.py 0.00% <ø> (ø)
... and 83 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 6a4c760...b5b26f0. Read the comment docs.

@isVoid
Copy link
Contributor Author

isVoid commented Jan 28, 2021

rerun tests

Copy link
Contributor

@brandon-b-miller brandon-b-miller left a comment

Choose a reason for hiding this comment

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

few typos otherwise LGTM

Co-authored-by: brandon-b-miller <[email protected]>
@galipremsagar galipremsagar added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Feb 4, 2021
@galipremsagar
Copy link
Contributor

rerun tests

@galipremsagar
Copy link
Contributor

@gpucibot merge

@isVoid
Copy link
Contributor Author

isVoid commented Feb 4, 2021

rerun tests

@rapids-bot rapids-bot bot merged commit fc9a00f into rapidsai:branch-0.18 Feb 4, 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. tests Unit testing for project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] assert_eq is failing to assert for integer comparison
3 participants