-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 InvariantMonoidal and FreeInvariantMonoidal #845
Conversation
Current coverage is 88.79%
@@ master #845 diff @@
==========================================
Files 228 232 +4
Lines 3012 3069 +57
Methods 2962 3017 +55
Messages 0 0
Branches 47 49 +2
==========================================
+ Hits 2674 2725 +51
- Misses 338 344 +6
Partials 0 0
|
Interesting. I’m not sure Also, are we going to have a |
6c2fc6f
to
a246ff0
Compare
@julienrf I think we are almost there. Unfolding your ×, these are the practical cases:
I think there would be no use cases of the other ones:
That being said ìt might be possible to factor out the implementation of |
a246ff0
to
51456ec
Compare
@@ -16,4 +16,13 @@ import simulacrum.typeclass | |||
def product[A, B](fa: F[A], fb: F[B]): F[(A, B)] | |||
} | |||
|
|||
object Cartesian extends CartesianArityFunctions | |||
object Cartesian extends CartesianArityFunctions with AlgebrCartesianInstances |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/AlgebrCartesian/AlgebraCartesian?
Hey @OlivierBlanvillain - sorry this took so long to review. I have similar concerns to Julien regarding having a bunch of Code-wise everything looks good modulo a couple typos and questions, but I'd like to hear other people's thoughts as well. @ceedubs ? |
I would argue that
|
Warning: rambling brain dump below. @OlivierBlanvillain thank you for this PR and like @adelbertc, I have to apologize for the radio silence for so long. Your code is high quality, but as mentioned it's tough to make decisions on what to include in cats and what not to. Some people have already complained about the cats-core jar being pretty large. I think that covariant forms of free (free monad, free applicative) are getting most of the attention in the FP Scala community right now, but I agree with you that the contravariant and invariant forms can also be really useful. This puts my brain into a spiral of "So do we include this? Do we break free structures into a separate module/library? Do we have different modules for different forms of free structures?". At the moment I think going back to having free in a separate module is most appealing to me, but we've already been there and back, so I'm afraid that might frustrate people. If I recall, one of the biggest reasons that we broke free into a separate module was for trampolining, which is now handled by That's a partial braindump of my thoughts. Clearly there are no answers in there. I think I'm pretty happy with any route forward from here. Would people hate me if I put together a proof of concept PR of separating out a module or two now that the cats landscape has shifted a bit thanks to |
51456ec
to
27e16ae
Compare
Sorry for not responding for so long but I started pondering a project yesterday that I think could really benefit from this. Now that free has its own module again, I'm less concerned about adding more structures to it. The next week is really busy for me, but I'm hoping to do a more thorough review of this in the near future. I'm also open to any naming ideas that people have. |
27e16ae
to
2dee07d
Compare
Cool :). I played a bit more with it (still in the context of codecs) and I have a cool example where the same "configuration" is used to generate a json parser, a json pretty printer, and a json schema. This example has quite a bit of boilerplate / manual wiring to combine unrelated algebras, but I have the feeling that this approach could lead to a neat alternative to automatic type class derivation entirely with cats. |
def combine(x: B, y: B): B = f(fa.combine(g(x), g(y))) | ||
} | ||
|
||
def pure[A](a: A): Semigroup[A] = new Semigroup[A] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a little strange. Does it make sense to have this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's wrong, but I'm not it's useful...
Should |
Yes, I think so. |
It would be great to also have @OlivierBlanvillain do you really need |
Won't this lead to a weird diamond inheritance with
Shall I do it in the same PR?
|
I think we could it afterward, let’s progress iteratively!
They are indeed equivalent once you add functor capabilities to your |
So these codecs (which are encode/decode really) are also encoded in Haskell as partial isomorphisms. See a example on this: Bidirectional Parsing - https://youtu.be/NHRIV7UNiPU?t=41m24s He points us to
What we are trying to achieve with |
Hey @Fristi :) I gave a superficial read to Invertible Syntax Descriptions a while ago, and indeed, they are trying to achieve the same thing WRT encoders/decoders. However, I think that the |
Hmm you are right. Introspection is not included. A partial iso is just two functions. To make a category of it you should use kleisli arrows to make them compose. Monoids are used to make choice between partial isos possible. I've skimmed through JsonGrammer2 sources and it appears he using a free category (see: https://github.com/MedeaMelana/JsonGrammar2/blob/master/src/Language/JsonGrammar/Grammar.hs#L40). As you can see he also lifts; category and monoid operations to the algebra. After that he just makes instances category and monoid instances for his algebra. This is exactly what we are doing at the moment. We also lift some operations of Monoidal and Invariant to a 'generic' and free algebra, but it has no real strong name (and that's something which itches for me ;p). He also uses stack prisms (which I haven't figured out yet, but it seems like some sort of heterogenous data structure to carry product type information). Monoids could be used to encode coproduct types in to these codecs I think. As a side note, something else I found: https://ocharles.org.uk/blog/posts/2014-06-10-reversible-serialization.html. Ollie Charles also did some work on this, have to read it though :) |
@Fristi @OlivierBlanvillain do you have a consensus on what the right way to go forward is? It seems like there's a lot of great stuff in this PR, so I don't want to hold it up forever, but if you have better ideas brewing, then we can of course put it on the back burner. |
@ceedubs I would say that the This is how I see things going forward:
|
@OlivierBlanvillain gotcha. That sounds good to me, and @mpilquist expressed interest in #802, so I suspect that he's on board. It looks like there are merge conflicts again. Could you please refrain from making unrelated formatting changes in future PRs -- it can lead to lots of merge conflicts and makes the diffs a little tougher to review. It'd be great if you could also adopt the new naming convention for implicit instances (#1061), but that can wait until a subsequent PR if you'd prefer. |
@ceedubs @OlivierBlanvillain Sounds fine by me! Let's keep this train going 👍 |
9ecdb34
to
01f82dd
Compare
I resolve the conflicts. All the formatting stuff is isolated in the first commit (which I rebased with a brutal |
} | ||
``` | ||
|
||
Given an instance of `InvariantMonoidal` for `Semigroup`, we are able to combine existing `Semigroup` instances to form a new `Semigroup` by using the `Catesian` syntax: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catesian
-> Cartesian
01f82dd
to
5d1b95d
Compare
@@ -83,4 +83,12 @@ object eq { | |||
implicit val unitEq: Eq[Unit] = new Eq[Unit] { | |||
def eqv(a: Unit, b: Unit): Boolean = true | |||
} | |||
|
|||
// To be removed once https://github.com/non/algebra/pull/125 is published | |||
implicit class EqAnd[A](self: Eq[A]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be needed any more.
5d1b95d
to
bed8b4f
Compare
bed8b4f
to
917d6af
Compare
*/ | ||
private[cats] sealed trait KernelCartesianInstances { | ||
implicit val catsInvariantSemigroup: Cartesian[Semigroup] = InvariantMonoidal.catsInvariantMonoidalSemigroup | ||
implicit val catsInvariantMonoid: Cartesian[Monoid] = InvariantMonoidal.catsInvariantMonoidalMonoid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since InvariantMonoidal
extends Cartesian
, can these just live in the InvariantMonoidal
companion object without also living here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, indeed it looks like scalac is able to find the ones in InvariantMonoidal
companion object!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I take that back, the tut
compilation fails because it can't find Cartesian[Semigroup]
with this change. I reverted it and made this commit for reference: OlivierBlanvillain@07d2c14
917d6af
to
2ab572a
Compare
- Rename Algebra*Instances → Kernel*Instances - Rename implicit instances - Typo in doc - Remove `implicit class EqAnd` (not needed any more)
2ab572a
to
aa2669b
Compare
I have one minor comment about an outdated reference to |
👍 It looks by and large good to me. Given the size of the PR, it certainly has space for further improvements (e.g. documentation on the free side), but I think those non-urgent improvements can come later. Would love to see people start profiting from this sooner rather than later. |
🎉 |
Nice work @OlivierBlanvillain 👍 🎉 |
Thanks all for your help! 😄 |
This PR adds
InvariantMonoidal
andFreeInvariantMonoidal
, the invariant analogous toApplicative
andFreeApplicative
. Following this, we could consider adding aMonoidal
type class and explore theContravariant
side of things.Edit: I'm done with this one, it's good for review (ping @Fristi & @mpilquist)!
I realized that for every instance of
InvariantCartesian
I had in mind its also possible to define apure
, so I reworked everything to define anInvariantMonoidal
andFreeInvariantMonoidal
instead of theirCartesian
counterpart.This is work in progress on #802.
Before I finish it (doc is surprisingly long to write 😄), I have concerns regarding the legitimacy of this new
InvariantCartesian
type class. I could not find new laws for it that cannot be proved by combining laws ofInvariant
andCartesian
, which makes me think thatInvariantCartesian
does nothing more than pairing anInvariant
with aCartesian
. As such, is it interesting at all? I'm pretty sure that theFreeInvariant
stuff (the original motivation for this PR), could be formulated directly in term ofInvariant
andCartesian
.Are there other example of type classes already in cats that add nothing more than "hey, I'm this one plus this one, that it!"? If we look at
Apply
as an example, is there a thing which has aFunctor
and aCartesian
, but not anApply
? If the answer is no, then why do we needApply
at all, given that all code using it could be rewritten in term ofFunctor
andCartesian
?I'm a bit confused, not really sure if my questions/concerns make sense here...