Improved integral shrink trees to match behavior of binary search

[TysonMN]

Fixes #224

This PR changes the implementation of Gen.integral so that the shrink tree corresponds to the choices that binary search would make.

This is a draft PR because more testing is needed. I "manually" tested it with 0 for destination and 4 for x, and the resulting tree matches my expectation, which I showed in #224 (comment).

I don't know how to create an automated test for this. The main problem is that calling Gen.integral includes randomness from the call to Random.integral. If I try to extract the createTree function to avoid that randomness, then I get these compile errors and warning.

error FS1114: The value 'Hedgehog.Gen.createTree' was marked inline but was not bound in the optimization environment
error FS1113: The value 'createTree' was marked inline but its implementation makes use of an internal or private function which is not sufficiently accessible
warning FS1116: A value marked as 'inline' has an unexpected value
error FS1118: Failed to inline the value 'createTree' marked 'inline', perhaps because a recursive value was marked 'inline'

The impression I get is that it is not possible to define a function that is both recursive and inlined. Is there a way to expose the createTree function for testing?

The code is not very pretty. For example, I reused Shrink.towards but then hacked the output to fit the one case I was testing. I wouldn't be surprised if I missed more edge cases, but I do think it is "mostly" correct. I would prefer to create some automated unit tests before simplifying the code.

Some good news is that all existing tests still pass.

[TysonMN]

Haskell Hedgehog now has its own version of this PR in hedgehogqa/haskell-hedgehog#413. Waiting for that to be completed before resuming work on this PR.

[TysonMN]

Work on this PR can resume because the corresponding PR hedgehogqa/haskell-hedgehog#413 in Haskell Hedgehog has been merged.

I will do this soon.

[TysonMN]

This PR is now ready for review.

I don't know how to create an automated test for this. The main problem is that calling Gen.integral includes randomness from the call to Random.integral. If I try to extract the createTree function to avoid that randomness, then I get these compile errors and warning.
[...]
The impression I get is that it is not possible to define a function that is both recursive and inlined. Is there a way to expose the createTree function for testing?
[...]
I wouldn't be surprised if I missed more edge cases, but I do think it is "mostly" correct. I would prefer to create some automated unit tests before simplifying the code.

Actually, it is easy to avoid the compile errors. Just define a local function to do the recursion and the containing function can be inlined.

I also added several tests. I am now confident that this code is bug-free.

After writing the tests, I found one bug in Shrink.towards when the inputs differ by 1, which I fixed.

The code is not very pretty. For example, I reused Shrink.towards but then hacked the output to fit the one case I was testing.

I now think the code is pretty. I modified Shrink.towards to give the perfect output for this new shrink tree construction.

This code is not a port of @HuwCampbell's code in PR hedgehogqa/haskell-hedgehog#413. I like this code better, and I think there is a chance that @HuwCampbell might like it more too based on this comment in our Slack conversation since this code is a recursive function on the values.

Part of it I think was that I started with a tree instead of a raw value up front.
Just how integral_ worked.
It meant I was building trees from trees, while just a recursive function on values would probably have been simpler.

[moodmosaic]

🏓 @HuwCampbell 👀 (You said, one day you might want to learn to read F#, I think this is a beautiful example to start.)

[moodmosaic]

💯

[TysonMN]

(I thought I already made this comment, but it must have been lost when GitHub gave me a 500 error.)

(For those watching the email, the first time the build failed was because I accidently had a compile error. I force pushed a fix for that.)

The fable-tests build is failing because of a 500 error while trying to obtain its GitHub Action. I was just able to download the Action using the link it he logs. Seems like we should just queue the build again.

[HuwCampbell]

This looks great.

[HuwCampbell]

It looks really good, I was planning on doing something like the createTree inside shrink as well.

[moodmosaic]

Seems like we should just queue the build again.

Done. All checks have passed now.

[moodmosaic]

@TysonMN, shall we merge this?

[TysonMN]

Yep, I think this is ready

[HuwCampbell]

I find the plus one / minus one a bit odd.

    let inline createTree (destination : ^a) (x : ^a) =
            let rec binarySearchTree (destination : ^a) (x : ^a) =
            let xs =
                towards destination x
                |> Seq.pairwise
                |> Seq.map (fun (d, x) -> binarySearchTree d x)
            Node (x, xs)
        if destination = x then
            Node (x, Seq.empty)
        else
            binarySearchTree destination x
            |> Tree.addChildValue destination

This looks semantically more similar to what I have in the Haskell version.

Edit: If this doesn't work for you I'm happy for it to go ahead as is.

[TysonMN]

I would definitely prefer not to be adding and subtracting 1.

Interesting though, there should never be an under or over flow.

Your code causes most of the tests to fail, but I see where you are going. I defined binarySearchTree d x so that the returned tree contains values in the interval [d, t]. You are suggesting that binarySearchTree d x should return a tree with values in the interval (d, t] = [d+1, t] (since we are only considering integral values).

I think that should work. Probably just have to adjust towards. I will work on this.

[TysonMN]

The behavior preserving transformation is to subtract one from all arguments passed in for destination and add one to all references to the parameter destination. That is this commit.

Good catch @HuwCampbell!

Now towards can probably be adjusted so that this function doesn't need the only remaining reference to one. I will work on that.

[TysonMN]

And what do ya know? The implementation of Shrink.towards that achieves the desired behavior is the one that already exists in master. I shouldn't have modified that function earlier in this branch.

Now the code is still correct and even prettier! Definitely time to merge :D

[HuwCampbell]

That's heaps better.

[HuwCampbell]

I defined binarySearchTree d x so that the returned tree contains values in the interval [d, t]. You are suggesting that binarySearchTree d x should return a tree with values in the interval (d, t] = [d+1, t] (since we are only considering integral values).

That's exactly right; and what my version in Haskell does. They are essentially identical now apart from the tree wrapping and unwrapping over there (which I aim to remove at some point).

[moodmosaic]

Great work and awesome @hedgehogqa org wide collaboration 🦔
@TysonMN 💯
@HuwCampbell 💯

[moodmosaic]

We should blog/write about this stuff one day!