-
Notifications
You must be signed in to change notification settings - Fork 107
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
Use binary shrinking for integral. #413
Conversation
ecd1c65
to
f9243a4
Compare
The current shrink strategy produces potentially quite a lot of duplication. By using a binary search, we should be able to significantly speed up shrinking. With this change we can see ``` Gen.printTreeWith 30 (Seed 5 3) $ Gen.int (Range.constant 0 22) 7 ├╼ 0 ├╼ 4 │ ├╼ 2 │ │ └╼ 1 │ └╼ 3 └╼ 6 └╼ 5 ``` While before we had ``` Gen.printTreeWith 30 (Seed 5 3) $ Gen.int (Range.constant 0 22) 7 ├╼ 0 ├╼ 4 │ ├╼ 0 │ ├╼ 2 │ │ ├╼ 0 │ │ └╼ 1 │ │ └╼ 0 │ └╼ 3 │ ├╼ 0 │ └╼ 2 │ ├╼ 0 │ └╼ 1 │ └╼ 0 └╼ 6 ├╼ 0 ├╼ 3 │ ├╼ 0 │ └╼ 2 │ ├╼ 0 │ └╼ 1 │ └╼ 0 └╼ 5 ├╼ 0 ├╼ 3 │ ├╼ 0 │ └╼ 2 │ ├╼ 0 │ └╼ 1 │ └╼ 0 └╼ 4 ├╼ 0 ├╼ 2 │ ├╼ 0 │ └╼ 1 │ └╼ 0 └╼ 3 ├╼ 0 └╼ 2 ├╼ 0 └╼ 1 └╼ 0 ``` The first level of the tree is exactly the same, but then the size of the tree reduces significantly as all duplication is removed. This is currently just for `integral`, but as integral is used for `element`, this should improve things pretty broadly.
f9243a4
to
a424a08
Compare
Quoting @HuwCampbell from Slack
Can you share some data that makes you think this? To debug in F#, in the test after all the values are generated, I would log/print the generated values. That is the true measure of how many tests are being run. It might be that the code by (Haskell) Hedgehog to count the number of tests is incorrect. |
I had a good hard stare at the shrink tree for genIllTyped and figured out what was happening. I was actually increasing the tree size if the generator started at 0. e.g.
instead of
The fix is pretty simple though, so I've added a commit for it. |
Very good. Is the number of shrinks for |
As far as I can tell yes. |
So I did some additional testing. This might be a little contrived, but I think it's useful nonetheless. import Data.IORef
import Control.Monad
import Control.Monad.IO.Class
import qualified Hedgehog.Gen as Gen
import qualified Hedgehog.Range as Range
counter <- Data.IORef.newIORef (0 :: Int)
forM [1..1000 :: Int] $ \_ ->
check . property $
do { x <- forAll (Gen.int (Range.constant 0 22));
liftIO $ Data.IORef.modifyIORef counter (+1);
assert (x < 1)
}
Data.IORef.readIORef counter Now according to hedgehog the number of shrinks is the same before and after, because it only counts a shrink as a successful descension of the tree; and this PR only elides non-fruitful checks which are never going to find a successful shrink. The good news is that the number of comparisons is significantly reduced. In this simple test, we're dropping from on average ~9.3 operations per So it's calling the function under test almost half as much. |
This is beautiful ✨ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. 👍 I think we can add a link to the URL of this PR (see suggestion) so we can easily get back to this in the future.
Also, it could be useful to have the code for the old way in the test suite, so we can compare the generators as they should always give identical results. This can be done separately, but preferably before the next release.
Exactly! Calling the function under test twice as many times as needed is exactly the behavior I noticed that started all this. See hedgehogqa/fsharp-hedgehog#224. |
Co-authored-by: Nikos Baxevanis <[email protected]>
The current shrink strategy produces potentially quite a lot of
duplication. By using a binary search, we should be able to
significantly speed up shrinking.
With this change we can see
While before we had
The first level of the tree is exactly the same, but then the size of the
tree reduces significantly as all duplication is removed.
This is currently just for
integral
, but as integral is used forelement
, this should improve things pretty broadly.