Moved functions from Random to Gen #238

ghost · 2020-11-10T06:40:00Z

Breaking change, but alluded to in #177

I wasn't able to get the exact type mentioned in that issue, and settled for the type found in this request.

type Gen<'a> = Gen of (Seed -> Size -> Tree<'a>)
// Needs to be this..?
type Gen<'a> = Gen of (Seed -> Size -> Tree<Option<'a>>)

Feel free to close if this is too big of a change.

ghost · 2020-11-11T21:12:45Z

There is a test failure because of the implementation of Gen.sampleTree. Any tips on what the implementation should look like are welcome.

TysonMN · 2020-11-11T22:08:48Z

There are many tests failing. I analyze just one of them here.

From this code to this code is not behavior preserving. Random.replicate splits the seed so that each of the count values is generated using a different seed. Your code generates each of the count values using the same seed. As such, the same value is generated every time. That is why this test (for all its parameterizations) is failing.

ghost · 2020-11-11T23:28:14Z

@TysonMN good catch, I used a local mutable (I know, ew) variable to split the seed for each iteration. I also had to add CompiledName attributes, but it all appears to be in order now, pending verification.

moodmosaic · 2020-11-12T11:18:13Z

Thank you, @adam-becker. Thank you, also, @TysonMN, for reviewing this in #238 (comment) as I haven't looked into the changes yet.

Question: Shall we get to this after merging the recheck feature in? Then, the plan is to make a new release, and then review this.

ghost · 2020-11-12T15:58:39Z

I also have a question. For this code base are we supposed to use Gen.unsafeRun when creating functions inside the Gen module?

ghost · 2020-12-03T19:08:59Z

@moodmosaic @TysonMN The branch for this PR has been rebased, it should be ready for review again.

TysonMN · 2021-01-03T02:29:27Z

Now that 0.9.0 is released, it is time to resume the review of this PR?

moodmosaic · 2021-01-04T13:56:22Z

Now that 0.9.0 is released, it is time to resume the review of this PR?

Yes. And perhaps we can pack this and #247 into the same (major, v10.x) release.

ghost · 2021-01-11T03:54:34Z

Rebased and ready for review again.

moodmosaic

👍

src/Hedgehog/CSharp/Gen.fs

src/Hedgehog/Gen.fs

moodmosaic · 2021-01-11T15:23:04Z

I just did a first round of review (I've only scanned the changes) and left a few nitpicks. Another round of review (looking at the actual impl of the moved functions) should be warranted. We should be good to go then.

I've also run Script.fsx, all the stuff in there worked as usual.

moodmosaic

Looks great. I left a comment around filter (the diff there isn't very clear, note that I'm reviewing this from the airport right now as I'm about to board on a plane).

src/Hedgehog/Gen.fs

moodmosaic · 2021-01-14T14:18:02Z

src/Hedgehog/Gen.fs

+                let g' = resize (2 * k + n) g
+                bind g' <| fun x ->
+                    if p x then
+                        constant (Some x)


It looks like the actual filtering part was removed. (?)

The filter part is still there, it's this part I believe (267-270):

if p x then constant (Some x) else tryN (k + 1) (n - 1)

I agree with @moodmosaic. Most of the changes in these lines are from renames or from merging modules. But where did the call to Tree.filter go?

If not calling Tree.filter is a bug, then we should make a test for it.

I agree with @TysonMN, it'd be nice with a test.

@moodmosaic @TysonMN I see what happened here. There were a few tryFilter* functions, tryFilterRandom and tryFilter. When Random was removed, the tryFilterRandom function was also removed. The code is close enough that the diff shows them as the same function that changed, when this isn't the case.

I believe we still need the functionality that tryFilterRandom exposed? Not sure what we want to name the function though.

src/Hedgehog/Gen.fs

moodmosaic · 2021-01-14T14:20:16Z

src/Hedgehog/Gen.fs

-                   |> Tree.filter (atLeast (Range.lowerBound size range))
-           })
+        bind (integral range) <| fun n ->
+            replicate n g


Will this implementation behave the same as the old/current one?

I'm not sure if it behaves the same, but I think it will behave as I would expect 😅 Basically just generate a number from the range, and generate that many elements as a list. As long as replicate has that behavior, and doesn't just duplicate the same element n times.

@moodmosaic I will add tests for this just to be sure, but I'm pretty confident as is.

I agree with @adam-becker. It is hard to know if the implementations are exactly the same, but the new implementation is "the right" one (because it doesn't do a deep dive into implementation details).

However, as I pointed out in other comments, a stack overflow bug was introduced in replicate.

src/Hedgehog/Gen.fs

TysonMN · 2021-01-15T13:14:42Z

src/Hedgehog/Gen.fs

+    let replicate (times : int) (g : Gen<'a>) : Gen<'a list> =
+        let rec loop n xs =
+            if n <= 0 then
+                constant xs
+            else
+                bind g (fun x -> loop (n - 1) (x :: xs))
+        loop times []


...while this recursion lacks tail calls. The stack will overflow if times is sufficiently large as in this code.

let n = 10000 Gen.constant 1 |> Gen.replicate n |> Gen.sample 0 1 |> ignore

This function nearly implements ListGen.sequence from PR #260. Implementing this function in terms of that one would only be slightly inefficient because of the additional call to List.rev. Even so, I suggest we first (correctly!...see below) implement traverse and sequence, and then implement this replicate in terms of sequence.

Furthermore, I now realize that traverse in PR #260 also lack tail calls. (I wrongly assumed that return! "magically" avoided this problem.)

The tricky bit with making this tail recursive is that you're multiple functors deep. List<Gen<'a>> where Gen<'a> returns a Tree<'a> for all of the shrinks. The Haskell version implements this with the replicateM function, but I think Haskell's laziness makes this work. Aside from that, this will be tricky and require some thought.

Probably the simplest thing to do is keep the implementation like it was before. We can keep this refactor in mind and come back to it later.

Actually, I am not sure it is possible to completely avoid stack overflows.

Recall that the type 'a -> 'b is (covariant) functor in 'b. The map function for this functor is function composition (which is either >> or << with only the order of inputs chanding). Our Random<'c> type is a (covariant) functor in 'c because it is just an wrapper around the type 'a -> 'b -> 'c, which is also a (covarant) functor in 'c. We can simplify things by uncurrying to get back to a function of the form 'a -> 'b.

I don't know a way to implement map for the (covariant) functor 'a -> 'b that won't overflow the stack. It might be impossible.

Specifically, the following test fails.

[<Fact>] let ``Does function composition overflow the stack? Answer: Yes`` () = let n = 100000 let f = id |> List.replicate n |> List.fold (>>) id f ()

With this in mind, I think we should be more willing to allow implementations that can overflow the stack like the one this comment thread is about. Changing the code to put more pressure on the stack will cause it to overflow sooner, but I currently think it is impossible to completely avoid in this case.

In conclusion, I suggest that we no longer hold up this PR because of this recursion that lacks tail calls. Perhaps we can put a comment in the code to remind us of that lack of tails calls.

TysonMN · 2021-01-15T13:16:25Z

src/Hedgehog/Random.fs

-    let replicate (times : int) (r : Random<'a>) : Random<List<'a>> =
-        Random <| fun seed0 size ->
-            let rec loop seed k acc =
-                if k <= 0 then
-                    acc
-                else
-                    let seed1, seed2 = Seed.split seed
-                    let x = unsafeRun seed1 size r
-                    loop seed2 (k - 1) (x :: acc)
-            loop seed0 times []


This recursion uses tail calls, while...

moodmosaic · 2021-01-23T19:51:49Z

@TysonMN happy to merge this if you think it's in a state that can be merged. I haven't followed after #238 (comment) because I'm still (partially) out and about.

TysonMN · 2021-01-23T20:05:22Z

@TysonMN happy to merge this if you think it's in a state that can be merged.

I would prefer to merge PR #266 first. I am completely convinced that removing Random is an improvement.

TysonMN · 2021-02-01T04:37:04Z

I am completely convinced that removing Random is an improvement.

Whoa! I meant to say that I am NOT completely convinced that removing Random is an improvement.

My current feeling is that we should not do this. Random is complicated enough, and it is only going to get more complicated as we fix stack overflow bugs like #289 and #177.

I think the separation between Gen and Random was/is not so good. I think this lead to the idea that things would be simplified by combining them. Instead, my guess is that things would get better if the separation were increased.

My current thinking is that Gen<'a> should essentially be Random<Tree<'a>> and the Gen module should contain mostly trivial functions that delegate to the Random and Tree modules.

I haven't focused directly on the separation between Gen and Random. Instead, I have been thinking about this indirectly while working adding features like ListGen.traverse and fixes for bugs like issue #289.

ghost · 2021-02-01T16:35:53Z

I am completely convinced that removing Random is an improvement.

Whoa! I meant to say that I am NOT completely convinced that removing Random is an improvement.

Can you expand on why? I see the little bits you mentioned later, but these seems more like an instinct rather than a firm judgment.

My current feeling is that we should not do this. Random is complicated enough, and it is only going to get more complicated as we fix stack overflow bugs like #289 and #177.

This is why I think removing this module/type will make things simpler going forward.

I think the separation between Gen and Random was/is not so good. I think this lead to the idea that things would be simplified by combining them. Instead, my guess is that things would get better if the separation were increased.

Not sure how much more they could be separated, they are distinct types/modules now.

My current thinking is that Gen<'a> should essentially be Random<Tree<'a>> and the Gen module should contain mostly trivial functions that delegate to the Random and Tree modules.

I think Gen<'a> should be the central type of this project. It seems like there are two currently, and that leads to confusion/bloat/needless complexity.

TysonMN · 2021-02-02T05:11:29Z

I think the separation between Gen and Random was/is not so good. I think this lead to the idea that things would be simplified by combining them. Instead, my guess is that things would get better if the separation were increased.

Not sure how much more they could be separated, they are distinct types/modules now.

The best example is probably Gen.bindRandom. It duplicates code from Random.bind.

In my study of functional programming, I found found a common way to implement bind for nested modands C<_> and D<_>, which is

module CD =
  let bind (f: 'a -> C<D<'b>>) : C<D<'a>> -> C<D<'b>> =
    f
    |> DC.traverse
    |> C.bind
    >> C.map D.flatten

I think Gen<'a> should be the central type of this project. It seems like there are two currently, and that leads to confusion/bloat/needless complexity.

I think that is because of duplicate code like in the above example. In only Random contained code specific to Random and Gen implemented everything for the nested monadic type Gen<_> without assuming the implementation details of those monadic types, then I don't think it would feel like there are two central types.

My current feeling is that we should not do this. Random is complicated enough, and it is only going to get more complicated as we fix stack overflow bugs like #289 and #177.

This is why I think removing this module/type will make things simpler going forward.

It will be easier to fix bugs specific to Random<_> when only working with Random<_> than when working with Random<Tree<_>>.

Whoa! I meant to say that I am NOT completely convinced that removing Random is an improvement.

Can you expand on why? I see the little bits you mentioned later, but these seems more like an instinct rather than a firm judgment.

Random<_> and Tree<_> are separate monads, so they should be implemented separately.

TysonMN · 2021-02-02T05:15:10Z

This PR makes more functions look like Gen.bindRandom. I think we should make more functions look Random.bind, Tree.bind, and the bind for nested monadic types that I gave in the previous comment.

TysonMN · 2021-02-02T14:26:32Z

Oh, by flatten, I mean the function that some call join, such as here.

fsharp-hedgehog/src/Hedgehog/Tree.fs

Lines 34 to 35 in 00e45b7

    
           let join (xss : Tree<Tree<'a>>) : Tree<'a> = 
        
               bind id xss

ghost · 2021-09-06T22:58:20Z

I've redone this branch entirely, Random is entirely gone. I also went through and streamlined Gen functions that were using Random. This extra layer of indirection is completely unnecessary, and a ton of extra allocations were removed. This should also help with Hedgehog's efficiency. I'm sure I missed a few opportunities to eliminate common sub-expressions, but I think this is ready for review again.

EDIT: Also, lots of functions are now tail-recursive. Including: Gen.list, Gen.filter and Gen.tryFilter

/cc @moodmosaic @TysonMN

TysonMN · 2021-09-06T23:46:59Z

I am still not convinced that this is a good idea. I will review the current branch and see if I come to a different conclusion.

ghost · 2021-09-11T18:03:45Z

Closing this for now. We can re-evaluate using the new contributing process.

TysonMN mentioned this pull request Nov 11, 2020

optimize test of withNull hedgehogqa/fsharp-hedgehog-experimental#41

Closed

ghost marked this pull request as draft November 11, 2020 21:12

ghost marked this pull request as ready for review November 11, 2020 23:28

TysonMN mentioned this pull request Nov 12, 2020

Generate lazily evaluated sequences #154

Closed

moodmosaic marked this pull request as draft December 1, 2020 11:39

ghost marked this pull request as ready for review December 8, 2020 22:58

TysonMN mentioned this pull request Jan 3, 2021

Incorrect shrinking (mutable objects in generator) #192

Closed

moodmosaic reviewed Jan 11, 2021

View reviewed changes

src/Hedgehog/CSharp/Gen.fs Outdated Show resolved Hide resolved

src/Hedgehog/Gen.fs Outdated Show resolved Hide resolved

src/Hedgehog/Gen.fs Outdated Show resolved Hide resolved

moodmosaic reviewed Jan 14, 2021

View reviewed changes

TysonMN requested changes Jan 15, 2021

View reviewed changes

TysonMN mentioned this pull request Jan 15, 2021

add ListGen.traverse #260

Merged

moodmosaic mentioned this pull request Jan 19, 2021

Release 0.10.0 #280

Closed

TysonMN mentioned this pull request Jan 24, 2021

Random.map without stack overflow #289

Open

ghost added this to the 0.11.0 milestone Jan 31, 2021

Adam Becker added 17 commits September 8, 2021 22:31

Inlined 'Random.delay'

111bd98

Inlined 'Random.tryFinally'

0ba41dc

Inlined 'Random.tryWith'

d184b1c

Inlined 'Random.map'

7395fd8

Inlined 'Random.sized'

f6506de

Inlined 'Random.Builder'

880882b

Inlined 'Random.double'

123dbd9

Inlined 'Random.resize'

5db1549

Inlined 'Random.constant'

bef8926

Inlined 'Random.integral'

37c7636

Inlined 'Random.bind'

2b974b1

Inlined 'Random.replicate'

d25978c

Gen/Random equivalence

918fbeb

Inlined 'Random.run'

7c1601e

Clean up after inlining

1b7e2c9

Make 'unsafeRun' private again.

5434ac8

Reintroduce 'mapTree'

67b93e3

ghost removed this from the 0.11.0 milestone Sep 11, 2021

ghost closed this Sep 11, 2021

ghost deleted the remove-random branch September 21, 2021 16:15

This pull request was closed.

Moved functions from Random to Gen #238

Moved functions from Random to Gen #238

Conversation

ghost commented Nov 10, 2020

ghost commented Nov 11, 2020

TysonMN commented Nov 11, 2020

ghost commented Nov 11, 2020

moodmosaic commented Nov 12, 2020

ghost commented Nov 12, 2020

ghost commented Dec 3, 2020

TysonMN commented Jan 3, 2021

moodmosaic commented Jan 4, 2021

ghost commented Jan 11, 2021

moodmosaic left a comment

Choose a reason for hiding this comment

moodmosaic commented Jan 11, 2021

moodmosaic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jan 15, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jan 14, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TysonMN Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

ghost Jan 15, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TysonMN Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

moodmosaic commented Jan 23, 2021

TysonMN commented Jan 23, 2021

TysonMN commented Feb 1, 2021

ghost commented Feb 1, 2021

TysonMN commented Feb 2, 2021 • edited Loading

TysonMN commented Feb 2, 2021

TysonMN commented Feb 2, 2021

ghost commented Sep 6, 2021 • edited by ghost Loading

TysonMN commented Sep 6, 2021

ghost commented Sep 11, 2021

ghost Jan 15, 2021 •

edited by ghost

Loading

ghost Jan 14, 2021 •

edited by ghost

Loading

TysonMN Jan 15, 2021 •

edited

Loading

ghost Jan 15, 2021 •

edited by ghost

Loading

TysonMN Jan 15, 2021 •

edited

Loading

TysonMN commented Feb 2, 2021 •

edited

Loading

ghost commented Sep 6, 2021 •

edited by ghost

Loading