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

Implement hypothesis strategies and tests for arrays #5017

Merged
merged 37 commits into from
Dec 8, 2022
Merged

Implement hypothesis strategies and tests for arrays #5017

merged 37 commits into from
Dec 8, 2022

Conversation

csadorf
Copy link
Contributor

@csadorf csadorf commented Nov 21, 2022

Implements hypothesis strategies for CumlArray and extends the test_array.py test module to test them and refactors existing tests to test against the strategies instead of parametrized inputs.

@github-actions github-actions bot added the Cython / Python Cython or Python issue label Nov 21, 2022
@csadorf csadorf added 2 - In Progress Currenty a work in progress improvement Improvement / enhancement to an existing function non-breaking Non-breaking change and removed Cython / Python Cython or Python issue labels Nov 21, 2022
@github-actions github-actions bot added the Cython / Python Cython or Python issue label Nov 22, 2022
@csadorf csadorf force-pushed the fea-hypothesis-stratgies-and-tests-for-arrays branch 2 times, most recently from 877e8eb to 4e645e8 Compare November 24, 2022 10:50
@csadorf csadorf requested a review from dantegd November 24, 2022 10:55
csadorf added 20 commits November 29, 2022 06:43
@csadorf
WIP: Implement hypothesis strategies and tests for arrays
d25d385
@csadorf
Remove explicit testing of numba arrays.
454a073
@csadorf
Continue implementation.
b5de8d4
@csadorf
Make create_cuml_array_input public function.
716e02f
@csadorf
Hypothesize test_get_set_item.
5ff60ad
@csadorf
Raise ValueError for invalid input to cuml_array_shapes.
7665b23
@csadorf
The cuml_array_shapes() strategy also returns integers.
f98739b
@csadorf
Only run standard number of examples.
5b990eb
@csadorf
Hypothesize test_create_empty.
3f35726
@csadorf
Reenable DeviceBuffer check since rapidsai#4332 is resolved.
03d0f0e
@csadorf
Remove obsolete py<38 compatibility work-around.
0b81196
@csadorf
Hypothesize test_create_* tests.
b8774ac
@csadorf
Improve shape normalization and inspection.
b214e77
@csadorf
Hypothesize test_output test.
b369b90
@csadorf
Hypothesize test_output_dtype test.
a698a23
@csadorf
Hypothesize test_cuda_array_interface test.
573916a
@csadorf
Hypothesize test_serialize test.
ccf730d
@csadorf
Hypothesize test_cumlary_binops and test_deepcopy tests.
751391a
@csadorf
Improve cuml_arrays strategy (currently not used).
0f95380
@csadorf
Cleanup test_array test module.
0782b4e 
Remove obsolete functions and constants.
@csadorf csadorf changed the title WIP: Implement hypothesis strategies and tests for arrays Implement hypothesis strategies and tests for arrays Nov 29, 2022
@csadorf
Copy link
Contributor Author

csadorf commented Nov 29, 2022

This is basically done, however I still want to take a moment to explore generating more arbitrary arrays – currently the strategies generate only ones exactly like the previous tests.

@csadorf
Copy link
Contributor Author

csadorf commented Nov 30, 2022

rerun tests

@csadorf
The cuml_array_inputs strategy generates more arbitrary arrays.
a399e85 
Requires some changes to the array tests.
@csadorf csadorf marked this pull request as ready for review November 30, 2022 18:39
@csadorf csadorf requested a review from a team as a code owner November 30, 2022 18:39
@csadorf csadorf added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currenty a work in progress labels Nov 30, 2022
wphicks
wphicks approved these changes Dec 1, 2022
View reviewed changes
Copy link
Contributor

@wphicks wphicks left a comment

Choose a reason for hiding this comment

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

Love it! Good implementation and an excellent improvement of the thoroughness of our testing generally.

@wphicks
Copy link
Contributor

wphicks commented Dec 1, 2022

@gpucibot merge

@csadorf csadorf added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Dec 1, 2022
@wphicks
Copy link
Contributor

wphicks commented Dec 2, 2022

rerun tests

@csadorf
Copy link
Contributor Author

csadorf commented Dec 6, 2022

Blocked by #5051 .

@csadorf csadorf added the 0 - Blocked Cannot progress due to external reasons label Dec 6, 2022
@csadorf
Copy link
Contributor Author

csadorf commented Dec 7, 2022

rerun tests

@csadorf csadorf removed the 0 - Blocked Cannot progress due to external reasons label Dec 7, 2022
@wphicks wphicks mentioned this pull request Dec 7, 2022
Merged
@wphicks
Copy link
Contributor

wphicks commented Dec 7, 2022

Looks like this failed with inconsistent test timings. @csadorf I don't see any harm in adding deadline=None to these, right?

@csadorf
Copy link
Contributor Author

csadorf commented Dec 8, 2022

Looks like this failed with inconsistent test timings. @csadorf I don't see any harm in adding deadline=None to these, right?

Yes, I will make that change.

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (branch-23.02@9f5ffea). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files 
@@               Coverage Diff               @@
##             branch-23.02    #5017   +/-   ##
===============================================
  Coverage                ?   79.81%           
===============================================
  Files                   ?      184           
  Lines                   ?    11786           
  Branches                ?        0           
===============================================
  Hits                    ?     9407           
  Misses                  ?     2379           
  Partials                ?        0
Flag Coverage Δ
dask 45.74% <0.00%> (?)
non-dask 69.42% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rapids-bot rapids-bot bot merged commit 276cb62 into rapidsai:branch-23.02 Dec 8, 2022
jakirkham pushed a commit to jakirkham/cuml that referenced this pull request Feb 27, 2023
@csadorf
Implement hypothesis strategies and tests for arrays (rapidsai#5017)
1106a26 
Implements hypothesis strategies for `CumlArray` and extends the `test_array.py` test module to test them and refactors existing tests to test against the strategies instead of parametrized inputs.

Authors:
  - Carl Simon Adorf (https://github.com/csadorf)

Approvers:
  - William Hicks (https://github.com/wphicks)

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

@wphicks wphicks wphicks approved these changes

@dantegd dantegd Awaiting requested review from dantegd

Assignees
No one assigned
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@csadorf @wphicks @codecov-commenter @dantegd