-
Notifications
You must be signed in to change notification settings - Fork 590
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 Hypothesis's ability to shrink sums #1403
Conversation
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.
Generally looks good to me, with a few minor comments. Merge at your own discretion 😄
(I also love the motivation for this change!)
|
||
@given(st.integers(), st.integers()) | ||
def test_does_not_exceed_100(m, n): | ||
assert m + n <= 100 |
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.
To fail with m=0, n=100
, this must use <
not <=
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.
Whoops, yes, thanks.
except OverflowError: | ||
return False | ||
return self.incorporate_new_buffer(attempt) | ||
if trial(m - 1, n + 1) and m > 1: |
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 think this should be m >= 1
, so that m
can be driven all the way to 0
, right? Also looks like putting the comparison before the call to trial
might avoid trying to incorporate a bad buffer. (these probably cancel each other in practice but I'd find it easier to follow the other way)
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.
No, this is correct. We know m > 0
. If m = 1
we want to run trial(0, n + 1)
and then stop regardless of what it returns. I'll add a comment to clarify the logic.
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.
Aaah, right. That does make sense, but a comment would certainly be good!
This improves Hypothesis's ability to shrink examples which depend on the sum of two values.
The motivating example for this is the following:
This fails because you can get an overflow if the sum is large and negative. The ideal minimum example for it in Hypothesis's ordering is
([], [], [], [-1], [-32768])
, but previously we rarely found that.This comes from the SmartCheck paper and I'm running some evaluations in part based on that paper and it annoyed me that Hypothesis didn't normalize example. This PR is part one of two in fixing that.