Speed up fair integer drawing #366
Conversation
|
I'm curious if you've seen this make an improvement in practice. I'm happy to merge it if you get the build green, but I suspect that it mostly doesn't help that much because of the way the bytes get generated meaning it rarely actually fails this check. |
|
Yeah, I made the change because I was running a test with hypothesis-print-statistics, and saw about equal numbers of valid and invalid examples. I dug in, and saw that this |
|
|
||
| probe = gap + 1 | ||
| while probe > gap: | ||
| probe = int_from_bytes(data.draw_bytes(nbytes, byte_distribution)) & mask |
There was a problem hiding this comment.
This line is currently too long (81 characters I think). If you run 'make format' and commit the changes it will fix that for you.
| probe = int_from_bytes(data.draw_bytes(nbytes, byte_distribution)) & mask | ||
|
|
||
| probe = gap + 1 | ||
| while probe > gap: |
There was a problem hiding this comment.
Because this loop always executes at least once by design, coverage is complaining about it. If you add a "# pragma: no branch" to it that will fix it.
|
Interesting. In that case I'm definitely happy to merge it! I've added some review comments explaining what you need to do to get a green build. |
cpennington
force-pushed
the
fast-integers
branch
from
d898c70
to
44a6e04
Compare
|
Ok, I've fixed the formatting/coverage issues. I think in general, re-drawing rather than declaring a sample invalid will lead to faster results, since it minimizes the amount of work that's re-done. Not sure if there's a good way to add catches for |
cpennington
force-pushed
the
fast-integers
branch
from
44a6e04
to
e58ebbd
Compare
This happens in a few places (e.g. composite does this for UnsatisfiedAssumption within it). There's a sort of balancing act to do with more complex situations though - sometimes aborting the test is the correct thing to do because otherwise you're going to keep retrying but never succeeding, stopping only when you hit the end of the buffer, which is potentially quite slow. In filter and composite I think he default is that it retries three times and then gives up. I agree that for small fast cases like this one you're probably right though. |
|
Yeah, that makes sense. |
| probe = int_from_bytes( | ||
| data.draw_bytes(nbytes, byte_distribution) | ||
| ) & mask | ||
|
|
||
| if probe <= gap: |
There was a problem hiding this comment.
Sorry, I misdiagnosed the coverage issue before. The actual problem is that this condition is now always satisfied because the previous bit loops until it is, so the if is now redundant.
Without this patch, drawing an integer outside the valid range marks the data as invalid, which forces a re-draw of all other data up to that point. After the patch, we immediately redraw the integer until it is within the target range. In practice, this doubled the number of valid examples found within a bounded time period.
cpennington
force-pushed
the
fast-integers
branch
from
e58ebbd
to
347ee5a
Compare
|
@DRMacIver: Can this merge now? |
|
It can! Sorry I didn't pick up on your last change sooner. Thanks! |
|
Great, thanks! |
Without this patch, drawing an integer outside the valid range marks the
data as invalid, which forces a re-draw of all other data up to that
point. After the patch, we immediately redraw the integer until it is
within the target range. In practice, this doubled the number of valid
examples found within a bounded time period.