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

Add concept random and concept exercise maze-maker #613

Merged
merged 20 commits into from
Oct 23, 2023
Merged

Conversation

jiegillet
Copy link
Contributor

@jiegillet jiegillet commented Oct 14, 2023

Closes #537

I had been thinking about doing this one for a while.
This is actually blocked by #549 (first time using fuzz tests) and exercism/elm-test-runner#49 (parsing fuzztests), but since I think there won't be big problems there and because I was motivated, I started working on this.

@pwadsworth if you are so inclined, I would love a review from you. If you could try to solve the exercise without looking at the exemplar and share your solution and impressions (is it too hard? too easy?) that would be great. As well as any other comment you might have of course.

The CI is expected to fail for two reasons: the test-runner will not be able to parse the exercise (EDIT: that's taken care of), and the introduction is missing from the exercise, I will copy it once it's been reviewed.

@@ -0,0 +1,125 @@
# Introduction
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to keep the intro and about almost exactly the same, because pretty much everything is needed for solving the exercise.
The only change I did is remove all the links, because they are considered distracting.

[ ( Test.Distribution.moreThanZero, "depth 0", \m -> MazeMakerSupport.mazeDepth m == 0 )
, ( Test.Distribution.moreThanZero, "depth 1", \m -> MazeMakerSupport.mazeDepth m == 1 )
, ( Test.Distribution.moreThanZero, "depth 2", \m -> MazeMakerSupport.mazeDepth m == 2 )
, ( Test.Distribution.moreThanZero, "depth 3", \m -> MazeMakerSupport.mazeDepth m == 3 )
Copy link
Contributor Author

@jiegillet jiegillet Oct 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The probability to get a depth of 3 is the smallest, just about 3%. Doing 1000 runs gives a >99.99% chance of that happening.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it is enough to test to depth 2 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mention the probability to get depth 3 in the next task, so I thought it would be nice to actually get it.

I thought 2 was a bit underwhelming, 1000 runs is really fast anyway, and a <0.01% chance of failure is reasonable.

Copy link
Contributor

@pwadsworth pwadsworth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The directions are clear and it helps that the requirements follow the same order as the concept's about.md. Here is my solution without looking at the exemplar. I did have to look at the hints in task 4 because the need to flatten the result, and how to do it, with Random.andThen identity were not obvious to me.

module MazeMaker exposing (..)

import Random exposing (Generator)


type Maze
    = DeadEnd
    | Room Treasure
    | Branch (List Maze)


type Treasure
    = Gold
    | Diamond
    | Friendship


deadend : Generator Maze
deadend =
    Random.constant DeadEnd


treasure : Generator Treasure
treasure =
    Random.uniform Gold [ Diamond, Friendship ]


room : Generator Maze
room =
    Random.map Room treasure


branch : Generator Maze -> Generator Maze
branch mazeGenerator =
    Random.int 2 4
        |> Random.andThen (\n -> Random.list n mazeGenerator)
        |> Random.map Branch


maze : Generator Maze
maze =
    Random.weighted ( 60, deadend )
        [ ( 15, room )
        , ( 25, branch (Random.lazy (\_ -> maze)) )
        ]
        |> Random.andThen identity


mazeOfDepth : Int -> Generator Maze
mazeOfDepth depth =
    if depth == 0 then
        Random.uniform deadend [ room ]
            |> Random.andThen identity

    else
        branch (Random.lazy (\_ -> mazeOfDepth (depth - 1)))

So how do we generate random values in Elm?
We split the problem in two: first, we describe the value that we want to generate with a `Random.Generator a`, then we generate a value.

The first way to generate a value is to create a `Random.Seed` via `initialSeed : Int -> Seed` and then use `step : Generator a -> Seed -> ( a, Seed )`, which returns a value and a new seed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First thoughts as I read the concept:

  • What a Seed is or does is not obvious. For me at least it required searching and reading to understand it in the context of a pure functional language. I would recommend providing a short description of what is a Seed and describe how it provides the starting point for a pure function to create a pseudo-random repeatable values, with a link to further reading.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll add a section about it in the about.md, but I'll keep it super short for the introduction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 233bd6f

Note that both of these functions are pure, so calling them twice with the same arguments will produce the same values.

The second way to generate a value is by using `generate : (a -> msg) -> Generator a -> Cmd msg`, but that can only be done inside an Elm application.
In that case, the Elm runtime may use `step` as well as outside, non-pure resources to generate seeds.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same points for Seed apply to the step concept.

exercises/concept/maze-maker/.docs/instructions.md Outdated Show resolved Hide resolved
concepts/random/about.md Outdated Show resolved Hide resolved
Random.uniform (Random.constant Zero)
[ Random.lazy (\_ -> peano) |> Random.map Next
]
|> Random.andThen identity
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of Random.andThen identity should be explained since it is necessary for step 4 of the exercise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I did explain what Random.andThen does, and I think it's pretty clear what identity is.
But really, I don't want to explain this too much, because I feel it's the only "challenging" part which requires a bit of thought. And it's not really that challenging either since I show it here used in exactly the same way and also I left a hint.
I'll think about it some more :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this code pretty confusing as well, I can't say I understand at the moment. I do understand identity, and andThen, so maybe its lazy that I am finding confusing. From the comment showing the output, it looks like generate 3 will create 3 uniformly distributed natural numbers between 0 and 2, is that right? And if so, what controls the range of the natural numbers produced?

Copy link
Contributor Author

@jiegillet jiegillet Oct 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this example is too strange? I was trying to find one that's simple enough and different enough from the exercise one. Let me know if you have a better idea.

First of all, lazydoesn't add anything to the logic, all it does is wrap the function in a lambda so that the compiler doesn't infinitely stick peano into peano.

Then, the type of Random.uniform (Random.constant Zero) [] is Generator (Generator Peano) since it picks from a list of generators. We need to flatten it with andThen : (a -> Generator b) -> Generator a -> Generator b, where a is Generator Peano and we want b to be Peano. So the identity function fits the job. (isn't this beautiful? That's one of the reasons I love Elm so much)

Lastly, what peano does is return Zero with a 50% chance , and itself plus one the other 50%. So the numbers coming out should be 0 (50% of the time), 1 (25% of the time), 2 (12% of the time), 3 (6% of the time), etc. The numbers are not bounded, because the generator could keep picking peano an arbitrary number of time, but it's more and more unlikely.

Maybe I should generate more numbers to get a better feel for the distribution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, that does make sense, and this explanation is great :)

Maybe add it to the document?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 2b0732d

exercises/concept/maze-maker/.docs/hints.md Outdated Show resolved Hide resolved
@ceddlyburge
Copy link
Contributor

Hi @jiegillet , I'm back from holiday now, do you want me to take a look at this?

@jiegillet
Copy link
Contributor Author

jiegillet commented Oct 18, 2023

Welcome back!
Yes, eventually I will want your opinion, but we should tackle the blocking PRs first.

Copy link
Contributor

@ceddlyburge ceddlyburge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, Random is a complex topic! It will be a great addition to the track.
I've left a few comments but nothing major.

concepts/random/about.md Outdated Show resolved Hide resolved
--> [1.61803, 3.14159, 2.71828]
```

Those values can be combined into pairs or lists of values:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Those values can be combined into pairs or lists of values:
Those values can be combined into lists of values, or tuples / pairs:

--> [Red, Blue, Blue, Green, Red]
```

`Random.uniform` takes two arguments to guarantee that there is at least one value to pick from.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`Random.uniform` takes two arguments to guarantee that there is at least one value to pick from.
`Random.uniform` takes two arguments (`Red, [Green, Blue]`) to guarantee that there is at least one value to pick from.
Otherwise if it took one list argument (`[Red, Green, Blue`]) then the list could be empty.

Random.uniform (Random.constant Zero)
[ Random.lazy (\_ -> peano) |> Random.map Next
]
|> Random.andThen identity
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this code pretty confusing as well, I can't say I understand at the moment. I do understand identity, and andThen, so maybe its lazy that I am finding confusing. From the comment showing the output, it looks like generate 3 will create 3 uniformly distributed natural numbers between 0 and 2, is that right? And if so, what controls the range of the natural numbers produced?

exercises/concept/maze-maker/.docs/hints.md Outdated Show resolved Hide resolved
## 5. We have to go deeper

- Use the [`Random.uniform`][random-uniform] function for generating the dead ends or rooms in on depth `0`
- If you end up defining a nested `Generator (Generator Maze)` generator, you can use the [`Random.andThen`][random-andThen] function to flatten it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe make this the same as the other similar comment, and mention identity.

exercises/concept/maze-maker/.meta/design.md Outdated Show resolved Hide resolved
@@ -0,0 +1,62 @@
module MazeMakerSupport exposing (..)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these are used in the exemplar / solution, so maybe its better for these to go in the test module? Maybe I'm missing something ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do mention MazeMakerSupport.mazeDepth in the instructions, so I thought it would be nice to show it.
Also, if I put those functions in the test file, the students will not be able to look at the definition at the same time as their failing test.
In general, I find that I almost never look at the test file since Exercism shows me the test snippet, so maybe this is more discoverable? I don't really know 🤷‍♂️

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think its a big deal either way, we could just wait for any feedback?

The only downside I see is that students might think they have to use it, being as its provided. I have no idea if anyone will think this though ...

[ ( Test.Distribution.moreThanZero, "depth 0", \m -> MazeMakerSupport.mazeDepth m == 0 )
, ( Test.Distribution.moreThanZero, "depth 1", \m -> MazeMakerSupport.mazeDepth m == 1 )
, ( Test.Distribution.moreThanZero, "depth 2", \m -> MazeMakerSupport.mazeDepth m == 2 )
, ( Test.Distribution.moreThanZero, "depth 3", \m -> MazeMakerSupport.mazeDepth m == 3 )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it is enough to test to depth 2 here?

@jiegillet
Copy link
Contributor Author

Ok, I think I've addressed all the comments. I'll merge it after the weekend in case someone wants to take a closer look.

@ceddlyburge
Copy link
Contributor

Looks good to me ...

@jiegillet jiegillet merged commit 3b1fa23 into main Oct 23, 2023
5 checks passed
@jiegillet jiegillet deleted the jie-random branch October 23, 2023 00:07
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.

Concept: Random
3 participants