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 documentation for Bimonad #4076

Merged
merged 11 commits into from
Dec 24, 2021
Merged

Add documentation for Bimonad #4076

merged 11 commits into from
Dec 24, 2021

Conversation

gatear
Copy link
Contributor

@gatear gatear commented Dec 8, 2021

The first draft for Bimonad docs as required in #1801
Couldn't find a better example .. i'm open to suggestions.

@gatear gatear mentioned this pull request Dec 8, 2021
70 tasks
@gatear gatear changed the title Bimonad docs Add documentation for Bimonad Dec 8, 2021

The `Bimonad` trait directly extends `Monad` and `Comonad` without introducing new behaviour. `Bimonad` is
different of other `Bi` typeclasses like `Bifunctor`, `Bifoldable` or `Bitraverse` where the prefix describes a
`F[_, _]`. The `Bimonad` is a `F[_]` and could be better seen as a dual monad i.e. something that is both a `Monad` and
Copy link
Member

Choose a reason for hiding this comment

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

"dual monad" is another one that I worry is confusing. For one, Comonad is not a Monad. Actually it is the dual of Monad!
https://typelevel.org/cats/typeclasses/comonad.html

Copy link
Contributor Author

@gatear gatear Dec 9, 2021

Choose a reason for hiding this comment

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

The DualMonad\SelfDualMonad thing i picked from old issues like #1297

I'll change it to:

The `Bimonad` is a `F[_]` and the `Bi` prefix has a different meaning here: it's both a `Monad` and a `Comonad`.

import cats.data._
import cats.implicits._

Bimonad[Eval].pure(true).extract === Eval.now(true).value
Copy link
Member

Choose a reason for hiding this comment

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

I think what you are trying to show here is that pure is implemented as now and extract as value? One alternative would be to define Bimonad[Eval] here just for demonstrative purposes, WDYT?

Copy link
Contributor Author

@gatear gatear Dec 9, 2021

Choose a reason for hiding this comment

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

I think what you are trying to show here is that pure is implemented as now and extract as value?

Yes

One alternative would be to define Bimonad[Eval] here just for demonstrative purposes, WDYT?

Something like that ?

val evalBimonad = Bimonad[Eval]

evalBimonad.pure(true).extract === Eval.now(true).value

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking like how they define optionMonad in the Monad docs:
http://typelevel.org/cats/typeclasses/monad.html

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! :)

import cats.implicits._

implicit def evalBimonad(implicit monad: Monad[Eval], comonad: Comonad[Eval]) =
new Bimonad[Eval] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Eval[_] is a bit of a bad choice here because the Comonad is not very safe. Calling .value on an Eval must be done with some care (it needs to be at O(1) stack depth or you lose the stack safety. If you are calling this as part of a recursive call it often blows up).

Id[_] is also a Comonad, but a bit of a boring one. I wonder if NonEmptyList[_] would be a better example since it is non-trivial but also not somewhat unsafe in this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm ok, I was hesitating between Eval[_] and NonEmptyList[_] for the main example.
I'll just use NonEmptyList[_].

@gatear gatear requested a review from johnynek December 9, 2021 20:10
//use the coflatMap from the Eval comonad
override def coflatMap[A, B](fa: Eval[A])(f: Eval[A] => B): Eval[B] =
//use the coflatMap from the NonEmptyList comonad
override def coflatMap[A, B](fa: NonEmptyList[A])(f: NonEmptyList[A] => B): NonEmptyList[B] =
comonad.coflatMap(fa)(f)
Copy link
Member

Choose a reason for hiding this comment

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

Actually we can directly call flatMap and coflatMap directly on the NonEmptyList :)

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 was hopping this shows flatMap and coflatMap as originating from monad and comonad.
Don't you think it's a good idea to delegate to the monad and comonad variables ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I do see that now. But then isn't it confusing, because pure and extract are also defined in monad and comonad, so we should delegate those as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.
I thought using one and head in the implementation will make this more obvious:

nelBimonad.pure(true).extract === NonEmptyList.one(true).head

Copy link
Member

Choose a reason for hiding this comment

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

I thought using one and head in the implementation will make this more obvious

I agree, I think this is the right way to do it. So, we can just delegate flatMap and coflatMap to the list without monad/comonad and maybe leave tailRecM as an exercise for the reader 😉

Anyway just my opinion, not really sure what the best thing to do here is. But I think separating the implementations of pure and extract makes it seem like they are inconsistent or different from the definitions in monad or comonad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the current tailRecM implementation for NonEmptyList[_] doesn't look trivial .. I mean I fear Its understanding detours too much and I agree it's a good idea to leave it as an exercise 👍

Anyway just my opinion, not really sure what the best thing to do here is. But I think separating the implementations of pure and extract makes it seem like they are inconsistent or different from the definitions in monad or comonad.

Yes, I will remove monad and comonad.

### NonEmptyList as a Bimonad
`NonEmptyList[_]` is a lawful `Bimonad` so you can chain computations (like a `Monad`) and `extract` the result at the end (like a `Comonad`).

Here is a possible implementation based on existing monad and comonad:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Here is a possible implementation based on existing monad and comonad:
Here is a possible implementation:

implicit def nelBimonad =
new Bimonad[NonEmptyList] {

//use NonEmptyList specific methods for creation and extraction
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a good place to point out the Bimonad law about the relationship between pure and extract? :)

Copy link
Contributor Author

@gatear gatear Dec 10, 2021

Choose a reason for hiding this comment

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

  //in order to have a lawful bimonad `pure` and `extract` need to respect: `nelBimonad.extract(nelBimonad.pure(a)) <-> a`

Something like this ?

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Thanks for iterating on all the feedback! Looking good, just a couple tiny things :)

implicit def nelBimonad =
new Bimonad[NonEmptyList] {

//in order to have a lawful bimonad `pure` and `extract` need to respect: `nelBimonad.extract(nelBimonad.pure(a)) <-> a`
Copy link
Member

Choose a reason for hiding this comment

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

Just a personal style nitpick but I prefer a space here between // and the comment 😅

Suggested change
//in order to have a lawful bimonad `pure` and `extract` need to respect: `nelBimonad.extract(nelBimonad.pure(a)) <-> a`
// in order to have a lawful bimonad `pure` and `extract` need to respect: `nelBimonad.extract(nelBimonad.pure(a)) <-> a`

Comment on lines 23 to 24
keep in mind `Bimonad` has its own added laws so something that is both monadic
and comonadic may not necessarily be a lawful `Bimonad`.
Copy link
Member

Choose a reason for hiding this comment

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

I think this is important enough to include at the beginning ...

The `Bimonad` trait directly extends `Monad` and `Comonad` without introducing new methods. `Bimonad` is
different from other `Bi` typeclasses like `Bifunctor`, `Bifoldable` or `Bitraverse` where the prefix describes a
`F[_, _]`. The `Bimonad` is a `F[_]` and the `Bi` prefix has a different meaning here: it's both a `Monad` and a `Comonad`.

Copy link
Member

Choose a reason for hiding this comment

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

... like this:

Suggested change
Keep in mind `Bimonad` has its own added laws so something that is both monadic
and comonadic may not necessarily be a lawful `Bimonad`.

armanbilge
armanbilge previously approved these changes Dec 13, 2021
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! :)

@gatear
Copy link
Contributor Author

gatear commented Dec 14, 2021

@armanbilge is this ready to merge or do i need 2 reviews ? I want to start working on another doc for Bifoldable after this one is merged

@armanbilge
Copy link
Member

Another pair of eyes is always good, maybe @johnynek can take another look at the next opportunity :)


If you use `Bimonad` as a convenience type such that:
```scala
def f[T[_] : Monad, Comonad, S](fa: T[S]): S
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be def f[T[_]: Monad: Comonad, S](fa: T[S]): S

I think the comma here is makingComonad just a type parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep my bad ..:Monad :Comonad is what I wanted

```
is re-written to:
```scala
def f[T[_] : Bimonad, S](fa: T[S]): S
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we generally don't put a space after the type, so we would change to: f[T[_]: Bimonad, S]

import cats.data._
import cats.implicits._

implicit def nelBimonad =
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make this a implicit val since it doesn't have any args.

@johnynek johnynek merged commit a818296 into typelevel:main Dec 24, 2021
@johnynek
Copy link
Contributor

Thanks!

@gatear gatear mentioned this pull request Jan 12, 2022
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.

None yet

3 participants