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

Sequence InvariantMonoidals #246

Merged
merged 5 commits into from
Oct 24, 2020
Merged

Conversation

vinayakpathak
Copy link

I don't think we need F[_] to be an Applicative to be able to sequence over it. InvariantMonoidal is all we need.

@joroKr21
Copy link
Member

joroKr21 commented Aug 23, 2020

Hmm, that is interesting. Thanks for the PR 👍
This is not binary compatible however. Instead of changing the current implicit definitions you can just add new ones (at lower priority).

@vinayakpathak
Copy link
Author

Yes, ok, I wasn't sure how to take care of the binary incompatibility. I'll try what you suggest and update

@joroKr21 joroKr21 added this to the 2.2.0 milestone Aug 30, 2020
@vinayakpathak
Copy link
Author

Ok, I've updated it. Seems like it's binary compatible now. I also added a few tests to test just the InvariantMonoidal part.

I'm not 100% sure that the way I've split things into different priorities is right. Please let me know if I've messed something up.

Copy link
Member

@joroKr21 joroKr21 left a comment

Choose a reason for hiding this comment

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

The inheritance relation between MkHNilSequencerLowPrio and MkHConsSequencerLowPrio also has to be inverted.

Thanks for the contribution! This is really complex stuff, but the use case with Semigroup looks really nice (although we already have derivation for that particular type class).

core/src/main/scala/cats/sequence/sequence.scala Outdated Show resolved Hide resolved
core/src/main/scala/cats/sequence/sequence.scala Outdated Show resolved Hide resolved
core/src/main/scala/cats/sequence/sequence.scala Outdated Show resolved Hide resolved
core/src/main/scala/cats/sequence/sequence.scala Outdated Show resolved Hide resolved
Copy link
Member

@joroKr21 joroKr21 left a comment

Choose a reason for hiding this comment

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

I made the changes because I'd like to merge and release 2.2.0 this weekend

@joroKr21 joroKr21 merged commit 35087e6 into typelevel:master Oct 24, 2020
@vinayakpathak
Copy link
Author

Oh, thanks for merging this!

My original motivation for this was to be able to write some appropriate typeclass instances for probability distributions. So you can define a Rand[T] class to represent a probability distribution and then you can define a monad instance for Rand. Combined with kittens, this gives you a great way to define joint distributions. For example, if you have a Rand[A] and a Rand[B] you can use the type-level sequence to get a Rand[A :: B :: HNil] for free, which is awesome!

However, with the previous code, this would only work if you had an Applicative instance for Rand. When doing probabilistic modelling, you face situations where all you have is a Density[A] (i.e., an unnormalized probability distribution) that simply gives you a def apply(a: A): Double. And now you want to be able to define joint densities instead. It is fairly straight-forward to define it mathematically (density over A and B is simply the product of density over A and density over B), but Density is not an applicative. This is what prompted me to make this change.

Ideally I would write my tests in terms of Density but that's not a data type in cats, so I decided to go with Semigroup instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants