-
Notifications
You must be signed in to change notification settings - Fork 30
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
Remove duplicates in frequency shrink tree #321
Remove duplicates in frequency shrink tree #321
Conversation
1a2f0ec
to
4c55af5
Compare
4c55af5
to
2177401
Compare
To be clear, I mean I think I was successful in fixing the bug. My implementation is probably not as elegant as @HuwCampbell's. I force pushed the branch with some new commits that added another test. This test ensures that the entire shrink tree produced by @HuwCampbell, although I was able to understand your Haskell code well enough, I don't understand it perfectly. Does your implementation produce balanced shrink trees? |
No it didn't produce a balanced tree. We did it before our other research on balanced trees. I think it should be possible though. |
Sure. The frequency shrink tree probably wasn't balanced back when then shrink trees included redundant values. What I meant was, is the shrink tree for a frequency generator in Haskell Hedgehog balanced (as defined above) now that shrinking happens without repetition? |
No it's not. It still uses the |
Maybe that can be your next improvement then. I am still curious, after that change, if the resulting tree is balanced. |
@TysonMN do you remember if this is ready to merge? |
I think so. As I recall, the tests here are very strong. |
Merged. 👍🏼 |
Fixes #319
We had the same bug that Haskell had. They fixed this bug in PR hedgehogqa/haskell-hedgehog#406. I tried to mimic their solution here. I think I was successful.
CC @HuwCampbell
I did one thing differently related to their
shrink
function. I think the inputn
of the lambda toshrink
is put into the root of the shrink tree byshrink
. Instead, I avoided an off-by-one error by first paring the weights and then callingtakeWhile
. Without pairing, the resulting sequence does not include the weight corresponding to the weightk
in the base case ofpick
.