Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Reduce pytest runtime #10203
Reduce pytest runtime #10203
Changes from 14 commits
4d7ae60
1f9e36d
70932d5
b94cce4
67e3994
e05d8bc
f367e01
9e37504
d298d4a
6e9a0ef
e4a98d0
289a13b
2e25ed2
9393bd0
a20102e
9c800cd
025c69d
60a0a92
6128b2a
227be3c
ecdb986
e3b6500
62d56be
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way to write this would be:
However you prefer is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I am just deleting tests that might be redundant. I am basing this off the assumption that all these different sets of numbers are not really increasing test coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some values of rtol and atol, the deleted values seem to be testing the behavior of
isclose
at a granularity finer than float32 (requiring float64s for accuracy). That seems potentially important, and would deserve a code comment explaining the coverage that each parameter set adds. Instead of deleting these, we might reframe these parameters so that we don't test a whole matrix of all input values and all atol/rtol values (6x6x6x6 = 1296 tests), and only test certain pieces of the matrix that actually cover the important pieces of the behavior (comparing ints and floats, testingisclose
at float64 precision, etc.).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT the these tests were only covering
float64
both before and after the change. The dataframe and array constructors implicitly upcast the incoming data, so it's always getting compared asfloat64
. Is your concern that we're not coveringfloat32
at all here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I wasn't very clear. The issue isn't really about data types. It's about the rtol / atol precision requiring
float64
precision, which these changes no longer test adequately. The real-world cases ofisclose
I have seen use very tight tolerances (sometimes tighter than float32 precision, like1e-08
for data on the order of1
). Currently, this PR removes the input data that is designed to test those cases of tight tolerances.If you look at the
data1
/data2
values like1.987654321
and1.9876543
, those pairs of input data are meant to be compared with the rtol/atol values of1e-08
in the other set of parameters. If we remove the more-precise values here, we aren't getting good coverage of the tighter tolerance1e-08
, which requiresfloat64
precision to get the correct results. By removing these pairings of parametrized values, this test would no longer fail if theisclose
functions in cuDF or cupy were to erroneously cast their inputs tofloat32
.I agree that this test is grossly over-parametrized, but the deletions here are removing an important case to check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just undo these changes and add a
TODO
that we can be more clever about parametrizing this particular test. The other changes in this PR give a more-than-meaningful improvement in test time and I don't think it's worth investing much more time over this single test at the moment.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that would be good. I can file an issue describing what I've discussed with @brandon-b-miller via Slack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me. Thank you, Bradley!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue #10284 filed. I can put this in my backlog of things to do, or I can help someone else construct the specific cases I have in mind for test coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change has been reverted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove the construction of GPU objects from the
parametrize
call? It occurs at collection time and is very expensive. This can be constructed lazily like:then:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I reason that that if
0:10
work then11:128
should too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe even replace
range(0, 10)
with[0, 1, 5, 10]
. Maybe even search the tests for the regexparametrize.*range
. 🙃