Make interleave logarithmtic rather than linear
#313
Conversation
The version of `interleave` before this PR was closely modelled on my blog post; a tiny bit _too_ closely; it wasn't the intention of the blog post to define "good" shrinkers. I had a footnote for that but should have made it a bit more prominent. Anyway, in this PR I change `interleave` from being linear (removing one element at a time) to logarithmic (trying to remove as big a chunk as possible at a time, trying the whole list, half the list, etc.), matching the behaviour of QuickCheck's `shrinkList`. This can have rather dramatic consequences for the performance of shrinking.
| (xs, _y, zs) <- splits ts | ||
| pure . TreeT . pure $ | ||
| interleave (xs ++ zs) | ||
| -- | @removes n@ computes all ways we can remove chunks of size @n@ from a list |
There was a problem hiding this comment.
I get what this is doing, but not quite sure it's clear from this sentence!
'all the ways we can remove chunks of size n' sounds like you'd have:
-- > removes 2 [1..4] = [[3,4],[1,4],[1,2]]
There was a problem hiding this comment.
Fair. Suggested rewording?
There was a problem hiding this comment.
Would "Splits a list into chunks of size n and returns all possible lists where one of these chunks has been removed" be less confusing?
There was a problem hiding this comment.
Again, I don't mind the behavior, but I can't reconcile how chunks of size 2 are removed from:
-- > removes 2 [1..5] == [[3,4,5],[1,2,5],[1,2,3,4]]
Ah ok, that is this part yes?
Note that the last chunk we delete might have fewer elements than
n.
Ok, I think I get it now, posting this ramble for posterity
There was a problem hiding this comment.
Yeah, it removed the chunk 5 : nothingMoreToRemove :)
| -- | ||
| -- Note that the last chunk we delete might have fewer elements than @n@. | ||
| removes :: forall a. Int -> [a] -> [[a]] | ||
| removes k = \xs -> go xs |
There was a problem hiding this comment.
Er, this should be eta reduced of course; this is a remnant of an earlier version.
There was a problem hiding this comment.
Do you want to do this? Happy with the rest
There was a problem hiding this comment.
| removes k = \xs -> go xs | |
| removes k = go |
| -- | ||
| -- Note that the last chunk we delete might have fewer elements than @n@. | ||
| removes :: forall a. Int -> [a] -> [[a]] | ||
| removes k = \xs -> go xs |
There was a problem hiding this comment.
Do you want to do this? Happy with the rest
| -- | ||
| -- Note that the last chunk we delete might have fewer elements than @n@. | ||
| removes :: forall a. Int -> [a] -> [[a]] | ||
| removes k = \xs -> go xs |
There was a problem hiding this comment.
| removes k = \xs -> go xs | |
| removes k = go |
| dropSome ts = do | ||
| n <- takeWhile (> 0) $ iterate (`div` 2) (length ts) | ||
| ts' <- removes n ts | ||
| pure . TreeT . pure $ interleave ts' |
There was a problem hiding this comment.
There's quite a lot going on here. It's not terribly obvious to me what the overall goal is and how each piece fits into it. Why chunk this way? Why choose these chunk sizes? How does the recursive call to interleave contribute to the whole job? Is there a way to track length information to avoid calling length repeatedly?
| @@ -320,7 +339,7 @@ interleave :: forall m a. Monad m => [NodeT m a] -> NodeT m [a] | |||
| interleave ts = | |||
There was a problem hiding this comment.
interleave is begging for documentation. Detailed documentation would be great; barring that, a clear statement of its general goals would be helpful.
The version of
interleavebefore this PR was closely modelled on myblog post; a tiny bit too closely; it wasn't the intention of the blog
post to define "good" shrinkers. I had a footnote for that but should
have made it a bit more prominent. Anyway, in this PR I change
interleavefrom being linear (removing one element at a time) tologarithmic (trying to remove as big a chunk as possible at a time,
trying the whole list, half the list, etc.), matching the behaviour of
QuickCheck's
shrinkList. This can have rather dramatic consequencesfor the performance of shrinking.