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.

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

I heard you like shrinking, so I shrunk the shrinker #2526

Merged
merged 5 commits into from
Aug 10, 2020

Conversation

DRMacIver
Copy link
Member

@DRMacIver DRMacIver commented Jul 31, 2020

It turns out that our test suite passes without a number of the shrink passes, saving only for some hyper-specific tests that tested specific behaviour of those passes. So I deleted them. The Hypothesis shrinker: Now with less code.

Motivation

Partly this is just because deleting code that we don't actually need is good and I encourage it, but I had some specific goals in mind for targeting these shrink passes: I have a low-key goal of removing the concept of examples from ConjectureData and making our code work purely based on integer values of blocks. Removing things that we don't need that rely on these features is a great first step towards that.

  1. We have too many uses of the word "example" in our code and doing a large scale refactoring which requires significant technical research is easier than coming up with a new name.
  2. The minithesis reducer has shown you can go remarkably far with this approach.
  3. The forthcoming reduction pass learning stuff will probably paper over any problems this causes.
  4. A lot of the tracking we do is very high overhead, and it would be nice to improve the performance of generation by removing that overhead.

(We're probably going to keep something like start_example and stop_example code so that we can use it for discards, but we hopefully won't need to maintain any sort of hierarchical tree of examples)

@DRMacIver DRMacIver force-pushed the DRMacIver/shrink-the-shrinker branch from 3230270 to 69da204 Compare July 31, 2020 11:16
@DRMacIver DRMacIver force-pushed the DRMacIver/shrink-the-shrinker branch 4 times, most recently from 64b908b to 952576a Compare August 4, 2020 12:51
@DRMacIver DRMacIver force-pushed the DRMacIver/shrink-the-shrinker branch from 952576a to 72cd8a2 Compare August 7, 2020 11:00
@DRMacIver
Copy link
Member Author

Going to take another executive "this probably doesn't need a review" decision.

@DRMacIver DRMacIver merged commit 34866c5 into master Aug 10, 2020
@DRMacIver DRMacIver deleted the DRMacIver/shrink-the-shrinker branch August 10, 2020 07:24
@Zac-HD
Copy link
Member

Zac-HD commented Aug 10, 2020

Agreed, but also feel free to ping me if you want a review - I'm very happy to, I'm just not tracking everything closely enough to judge when it would be helpful 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants