Change frequency generator to immediately shrink #406

ocharles · 2020-10-14T14:03:09Z

Change frequency generator immediately shrink

The frequency generators works by totalling all weights, and then
picking a random number between [0, total]. It then selects the first
generator who's cummulative weight matches this number. This works fine,
but the current implementation leads to poor shrinking behavior. In
particular, the shrinking is driven by repeatedly picking smaller
numbers in the range [0, total]. For skewed distributions, this can mean
repeatedly reselecting the same generator.

This commit makes the observation that the weight only matters for the
first pick. Once you have made a choice and observed a test failure, you
can give all generators equal weight, as we're going to perform a
breadth-first enumeration to try and find a shrink anyway.

Before this patch, we would see behavior like:

> printWith 30 (Seed 4 1) $ Hedgehog.Gen.maybe alphaNum
=== Outcome ===
Just 'o'
=== Shrinks ===
Nothing
Just 'o'
Just 'o'
Just 'o'
Just 'o'
Just 'a'
Just 'h'
Just 'l'
Just 'n'

Note that Just 'o' appears many times - this is because the maybe
generator puts a heavy bias towards 'Just' constructors.

With this commit, we get the much more minimal output:

> printWith 30 (Seed 4 1) $ Hedgehog.Gen.maybe alphaNum
=== Outcome ===
Just 'o'
=== Shrinks ===
Nothing
Just 'a'
Just 'h'
Just 'l'
Just 'n'

Note that no characters were duplicated.

TysonMN · 2020-10-14T14:29:05Z

Why does the outcome change? (Just 'o' vs Just 'I')

ocharles · 2020-10-14T16:09:24Z

Oh, I meant to mention - I think this changes how the seed is used. I tried using a stable seed, but for some reason you get different results (though it's still stable). Does that make sense?

Basically, using seed=x on master gives different results to my branch, but repeatedly running my branch with seed=x will give the same result each time.

Edit: I think this is happening because of this new line:

gens <- traverse toTreeMaybeT (upTo n xs0)

which will end up splitting the seed for the call to gen

HuwCampbell · 2020-10-14T16:34:04Z

hedgehog/src/Hedgehog/Internal/Gen.hs

+
+    -- Pick a single generator from the weighted list of generators. We prune
+    -- here as shrinks no longer need to care about bias.
+    n <- prune $ integral $ Range.constant 1 total


Instead of integral and prune, just use integral_.

Thanks, I didn't know about that!

HuwCampbell · 2020-10-14T16:58:58Z

Thanks Ollie. Cool investigation.

I've had a play and I think there's a slightly neater solution which doesn't require the tree hacking.

The intuition is the same, but just uses a modified shrink on integral to not include the duplicates.

frequency :: MonadGen m => [(Int, m a)] -> m a
frequency = \case
  [] ->
    error "Hedgehog.Gen.frequency: used with empty list"
  xs0 -> do
    let
      pick n = \case
        [] ->
          error "Hedgehog.Gen.frequency/pick: used with empty list"
        (k, x) : xs ->
          if n <= k then
            x
          else
            pick (n - k) xs

      iis =
        scanl1 (+) (fmap fst xs0)

      total =
        last iis

    n <- shrink (\n -> takeWhile (< n) iis) $ integral_ $ Range.constant 1 total
    pick n xs0

What do you think?

~~Edit: the list might be best reversed?~~

ocharles · 2020-10-14T17:00:48Z

That sounds great! I certainly wasn't happy about how many guts I had to open up to get this to work. I'll have a play around with your code tomorrow and see what I can make of it.

TysonMN · 2020-10-14T20:48:29Z

I think this changes how the seed is used. I tried using a stable seed, but for some reason you get different results (though it's still stable). Does that make sense?

Basically, using seed=x on master gives different results to my branch, but repeatedly running my branch with seed=x will give the same result each time.

Yes, that makes sense. However, I don't understand why each case stopped where it did.

In your example, do all values correspond to a failed test? That is, does the shrinking keep happening until there is nothing left to shrink (not because the test passed)?

ocharles · 2020-10-14T20:50:32Z

@TysonMN what you're seeing above is the output of print - https://hackage.haskell.org/package/hedgehog-1.0.3/docs/Hedgehog-Gen.html#v:print. This shows just one shrinking path. Perhaps you would like to see the output of printTree for an exhaustive enumeration of all shrinks? The way Hedgehog shrinking works is essentially a depth first traversal of the shrink tree, for as long as the test fails. The moment a test starts suceeding, we back track to the last failure and take the next alternative.

I forget exactly what print does, but I think it's essentially the left-most branch of the shrink tree.

ocharles · 2020-10-15T12:22:15Z

@HuwCampbell Thanks for your suggestion, it looks like it works great!

> printTree $ frequency [(1, pure "a"), (3, pure "b"), (10, show <$> bool)]
"True"
 ├╼"a"
 ├╼"b"
 │  └╼"a"
 └╼"False"

Originally, it could be

"True"
 ├╼"a"
 ├╼"True"
 │  ├╼"a"
 │  ├╼"b"
 │  │  ├╼"a"
 │  │  └╼"b"
 │  │     └╼"a"
 │  ├╼"b"
 │  │  ├╼"a"
 │  │  ├╼"b"
 │  │  │  └╼"a"
 │  │  └╼"b"
 │  │     ├╼"a"
 │  │     └╼"b"
 │  │        └╼"a"
 │  └╼"False"
 ├╼"True"
 │  ├╼"a"
 │  ├╼"b"
 │  │  ├╼"a"
 │  │  ├╼"b"
 │  │  │  └╼"a"
 │  │  └╼"b"
 │  │     ├╼"a"
 │  │     └╼"b"
 │  │        └╼"a"
 │  ├╼"True"
 │  │  ├╼"a"
 │  │  ├╼"b"
 │  │  │  ├╼"a"
 │  │  │  └╼"b"
 │  │  │     └╼"a"
 │  │  ├╼"True"
 │  │  │  ├╼"a"
 │  │  │  ├╼"b"
 │  │  │  │  ├╼"a"
 │  │  │  │  └╼"b"
 │  │  │  │     └╼"a"
 │  │  │  ├╼"b"
 │  │  │  │  ├╼"a"
 │  │  │  │  ├╼"b"
 │  │  │  │  │  └╼"a"
 │  │  │  │  └╼"b"
 │  │  │  │     ├╼"a"
 │  │  │  │     └╼"b"
 │  │  │  │        └╼"a"
 │  │  │  └╼"False"
 │  │  └╼"False"
 │  └╼"False"
 ├╼"True"
 │  ├╼"a"
 │  ├╼"b"
 │  │  ├╼"a"
 │  │  ├╼"b"
 │  │  │  └╼"a"
 │  │  └╼"b"
 │  │     ├╼"a"
 │  │     └╼"b"
 │  │        └╼"a"
 │  ├╼"True"
 │  │  ├╼"a"
 │  │  ├╼"b"
 │  │  │  ├╼"a"
 │  │  │  └╼"b"
 │  │  │     └╼"a"
 │  │  ├╼"True"
 │  │  │  ├╼"a"
 │  │  │  ├╼"b"
 │  │  │  │  ├╼"a"
 │  │  │  │  └╼"b"
 │  │  │  │     └╼"a"
 │  │  │  ├╼"b"
 │  │  │  │  ├╼"a"
 │  │  │  │  ├╼"b"
 │  │  │  │  │  └╼"a"
 │  │  │  │  └╼"b"
 │  │  │  │     ├╼"a"
 │  │  │  │     └╼"b"
 │  │  │  │        └╼"a"
 │  │  │  └╼"False"
 │  │  └╼"False"
 │  ├╼"True"
 │  │  ├╼"a"
 │  │  ├╼"b"
 │  │  │  ├╼"a"
 │  │  │  ├╼"b"
 │  │  │  │  └╼"a"
 │  │  │  └╼"b"
 │  │  │     ├╼"a"
 │  │  │     └╼"b"
 │  │  │        └╼"a"
 │  │  ├╼"True"
 │  │  │  ├╼"a"
 │  │  │  ├╼"b"
 │  │  │  │  ├╼"a"
 │  │  │  │  └╼"b"
 │  │  │  │     └╼"a"
 │  │  │  ├╼"True"
 │  │  │  │  ├╼"a"
 │  │  │  │  ├╼"b"
 │  │  │  │  │  ├╼"a"
 │  │  │  │  │  └╼"b"
 │  │  │  │  │     └╼"a"
 │  │  │  │  ├╼"b"
 │  │  │  │  │  ├╼"a"
 │  │  │  │  │  ├╼"b"
 │  │  │  │  │  │  └╼"a"
 │  │  │  │  │  └╼"b"
 │  │  │  │  │     ├╼"a"
 │  │  │  │  │     └╼"b"
 │  │  │  │  │        └╼"a"
 │  │  │  │  └╼"False"
 │  │  │  └╼"False"
 │  │  └╼"False"
 │  └╼"False"
 └╼"False"

so this is much better!

HuwCampbell · 2020-10-15T22:13:16Z

Yeah I'm pretty happy with the result. It does address the duplicates seen in #118 as well.

HuwCampbell · 2020-10-17T10:03:51Z

@ocharles can you squash this to one commit with both of us as authors?

ocharles · 2020-10-17T10:23:23Z

@HuwCampbell How does that look?

HuwCampbell · 2020-10-17T10:26:11Z

Thanks! that second shrink example is probably off now though.

ocharles · 2020-10-17T10:27:12Z

I think it's still pretty reasonable, it might not be exactly right, but it illustrates that there are no duplicates. I think the only material difference is that Nothing happens first, rather than last.

HuwCampbell · 2020-10-17T10:43:35Z

I only mentioned because it was brought up that the initial value changed, while that's no longer the case.

ocharles · 2020-10-17T10:44:01Z

Ah good point, I'll generate a new output.

The frequency generators works by totalling all weights, and then picking a random number between [0, total]. It then selects the first generator who's cummulative weight matches this number. This works fine, but the current implementation leads to poor shrinking behavior. In particular, the shrinking is driven by repeatedly picking smaller numbers in the range [0, total]. For skewed distributions, this can mean repeatedly reselecting the same generator. This commit makes the observation that the weight only matters for the first pick. Once you have made a choice and observed a test failure, you can give all generators equal weight, as we're going to perform a breadth-first enumeration to try and find a shrink anyway. Before this patch, we would see behavior like: ``` > printWith 30 (Seed 4 1) $ Hedgehog.Gen.maybe alphaNum === Outcome === Just 'o' === Shrinks === Nothing Just 'o' Just 'o' Just 'o' Just 'o' Just 'a' Just 'h' Just 'l' Just 'n' ``` Note that `Just 'o'` appears many times - this is because the maybe generator puts a heavy bias towards 'Just' constructors. With this commit, we get the much more minimal output: ``` > printWith 30 (Seed 4 1) $ Hedgehog.Gen.maybe alphaNum === Outcome === Just 'o' === Shrinks === Nothing Just 'a' Just 'h' Just 'l' Just 'n' ``` Note that no characters were duplicated. Co-authored-by: Huw Campbell <huw.campbell@gmail.com>

ocharles · 2020-10-17T10:50:37Z

Commit and pull request updated ✍️

moodmosaic

Looks good 👍 Great collab work as well 🎸

ocharles changed the title ~~Change frequency generator immediately shrink~~ Change frequency generator to immediately shrink Oct 14, 2020

HuwCampbell reviewed Oct 14, 2020

View reviewed changes

ocharles force-pushed the shrink-frequency branch from e2a63eb to 628ff8e Compare October 17, 2020 10:23

ocharles force-pushed the shrink-frequency branch from 628ff8e to 4dba9df Compare October 17, 2020 10:24

ocharles force-pushed the shrink-frequency branch from 4dba9df to b724ecc Compare October 17, 2020 10:50

moodmosaic approved these changes Oct 20, 2020

View reviewed changes

moodmosaic merged commit 7030059 into hedgehogqa:master Oct 20, 2020

TysonMN mentioned this pull request Oct 30, 2020

printTree function hedgehogqa/fsharp-hedgehog#227

Closed

This was referenced Nov 9, 2020

Same input repeatedly tested during shrinking hedgehogqa/fsharp-hedgehog#224

Closed

Duplicates in shrink trees for maybe #118

Closed

This was referenced Feb 22, 2021

Frequency generator should immediately shrink hedgehogqa/fsharp-hedgehog#319

Closed

Remove duplicates in frequency shrink tree hedgehogqa/fsharp-hedgehog#321

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change frequency generator to immediately shrink #406

Change frequency generator to immediately shrink #406

ocharles commented Oct 14, 2020 •

edited

Loading

TysonMN commented Oct 14, 2020

ocharles commented Oct 14, 2020 •

edited

Loading

HuwCampbell Oct 14, 2020

ocharles Oct 14, 2020

HuwCampbell commented Oct 14, 2020 •

edited

Loading

ocharles commented Oct 14, 2020

TysonMN commented Oct 14, 2020 •

edited

Loading

ocharles commented Oct 14, 2020

ocharles commented Oct 15, 2020

HuwCampbell commented Oct 15, 2020

HuwCampbell commented Oct 17, 2020

ocharles commented Oct 17, 2020

HuwCampbell commented Oct 17, 2020

ocharles commented Oct 17, 2020

HuwCampbell commented Oct 17, 2020

ocharles commented Oct 17, 2020

ocharles commented Oct 17, 2020

moodmosaic left a comment

Change frequency generator to immediately shrink #406

Change frequency generator to immediately shrink #406

Conversation

ocharles commented Oct 14, 2020 • edited Loading

TysonMN commented Oct 14, 2020

ocharles commented Oct 14, 2020 • edited Loading

HuwCampbell Oct 14, 2020

Choose a reason for hiding this comment

ocharles Oct 14, 2020

Choose a reason for hiding this comment

HuwCampbell commented Oct 14, 2020 • edited Loading

ocharles commented Oct 14, 2020

TysonMN commented Oct 14, 2020 • edited Loading

ocharles commented Oct 14, 2020

ocharles commented Oct 15, 2020

HuwCampbell commented Oct 15, 2020

HuwCampbell commented Oct 17, 2020

ocharles commented Oct 17, 2020

HuwCampbell commented Oct 17, 2020

ocharles commented Oct 17, 2020

HuwCampbell commented Oct 17, 2020

ocharles commented Oct 17, 2020

ocharles commented Oct 17, 2020

moodmosaic left a comment

Choose a reason for hiding this comment

ocharles commented Oct 14, 2020 •

edited

Loading

ocharles commented Oct 14, 2020 •

edited

Loading

HuwCampbell commented Oct 14, 2020 •

edited

Loading

TysonMN commented Oct 14, 2020 •

edited

Loading