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

Remove duplicates in frequency shrink tree #321

Merged
5 commits merged into from
Aug 20, 2021

Conversation

TysonMN
Copy link
Member

@TysonMN TysonMN commented Feb 23, 2021

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 input n of the lambda to shrink is put into the root of the shrink tree by shrink. Instead, I avoided an off-by-one error by first paring the weights and then calling takeWhile. Without pairing, the resulting sequence does not include the weight corresponding to the weight k in the base case of pick.

@TysonMN TysonMN force-pushed the 319_fix_frequency_shrinking_duplicates branch from 1a2f0ec to 4c55af5 Compare February 23, 2021 17:10
@TysonMN TysonMN force-pushed the 319_fix_frequency_shrinking_duplicates branch from 4c55af5 to 2177401 Compare February 23, 2021 17:34
@TysonMN
Copy link
Member Author

TysonMN commented Feb 23, 2021

I tried to mimic their solution here. I think I was successful.

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 frequency is balanced. Because a test failure means recursing into the corresponding subtree and a test success means moving on to the next child, a balanced shrink tree is one in which, at each subtree/Node, the number of children and the depth of that subtree differ by at most 1.

@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?

@HuwCampbell
Copy link
Member

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.

@TysonMN
Copy link
Member Author

TysonMN commented Mar 9, 2021

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?

@HuwCampbell
Copy link
Member

No it's not.

It still uses the shrink function, so there's a small amount of duplication there.

@TysonMN
Copy link
Member Author

TysonMN commented Mar 10, 2021

Maybe that can be your next improvement then. I am still curious, after that change, if the resulting tree is balanced.

@ghost ghost added this to the 0.11.0 milestone Mar 24, 2021
@ghost
Copy link

ghost commented Aug 16, 2021

@TysonMN do you remember if this is ready to merge?

@TysonMN
Copy link
Member Author

TysonMN commented Aug 16, 2021

I think so. As I recall, the tests here are very strong.

@ghost ghost merged commit 8dcc09f into hedgehogqa:master Aug 20, 2021
@ghost
Copy link

ghost commented Aug 20, 2021

Merged. 👍🏼

@TysonMN TysonMN deleted the 319_fix_frequency_shrinking_duplicates branch September 14, 2021 01:13
This pull request was closed.
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.

Frequency generator should immediately shrink
2 participants