Remove some problematic shrinker passes #461
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hmm. Looks like there are a bunch of shrink quality checks elsewhere that raise problems with this. I'll tinker with this further. |
The later per index shrinks can achieve the main purpose of this shrink (subtracting one from individual values) but are less likely to get stuck in the problem of repeatedly performing small shrinks because they try shrinking to smaller values first.
This no longer seems to be needed (I'm not sure why - some other changes to the minimizer seem to have supplanted it) to pass the example quality tests, and it's quite an expensive pass which usually doesn't work, so let's just remove it.
This test only existed to trigger the interval replacement code more reliably because it was hard to come up with a direct example that did.
This basically does a partial insertion sort on blocks of the same length, moving cheaper blocks earlier in the process. This helps to achieve a canonical result rather than the messier ones we might otherwise get. This is something I tried before but wasn't useful then. It is now - the interval replacement largely did the job that this is now doing, but more expensively.
DRMacIver
force-pushed
the
DRMacIver/harden-large-union-test
branch
from
79b7fe6
to
4342e8c
Compare
alexwlchan
approved these changes
Feb 16, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This removes two passes from the shrinker:
These turn out to be the source of at least one flaky test in test_example_quality (the large union test) - both of them would occasionally get stuck there and produce bad shrinks:
Moreover both of these passes appear to be unneeded. The ddsub pass is mostly redundant with the later attempts to shrink individual indices, which work better because they tend to go straight for a larger shrink by starting from close to 0.
I'm not sure why the interval replacement pass is now unneeded, but all the example quality tests pass without it. It seems to be the result of improvements in the rest of the shrinker now making it largely redundant.