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 Align typeclass (#1263) #1755

Closed
wants to merge 2 commits into from

Conversation

julianmichael
Copy link

@julianmichael julianmichael commented Jul 2, 2017

Addresses #1263.

Design taken from the Data.Align Haskell package. Implementing this raised some issues:

  • If we have both FunctorFilter and Align, we have a reasonable implementation of zip and zipWith. I haven't tried to prove it, but you might even be able to show that the resulting implementation of zip satisfies the reasonable law of being a left inverse to unzip.
  • nil seems redundant with empty in MonadFilter and MonoidK. Probably it should also be called empty... but notably it just adds to the weirdness of having several different "empties", some of which are forced to converge to the same thing. On that note, we probably want another law saying that these empties have to coincide.

It seems there has been some discussion both of adding empty to FunctorFilter and removing FunctorFilter entirely. I think it might be nice to have a Zip typeclass, in which case retaining FunctorFilter might be useful for relating the two, though maybe that should just be done with MonadCombine or something.

On the subject of zipping, if in the future we wished to have Naperian functors (which I think would be nice too, though maybe that's more the kind of thing to leave in a third-party lib), then Align would only be able to serve as a generalization of it if we removed nil from the typeclass. Furthermore, in the Haskell documentation there is the observation that an Align instance makes a functor F lax monoidal wrt the cartesian monoidal structure on Kleisli[Option, ?, ?] if the functor already admits a function (A -> Option[B]) -> F[A] -> F[B], which basically means FunctorFilter. But in that case, we basically already get our nil value too; you just need to ensure the extra law that nil is an identity of align.

So maybe it'd be best to remove nil from Align and add a Zip typeclass that inherits from it and FunctorFilter, which adds empty and the relevant laws as well as zip and zipWith. This captures the familiar notion of zipping from lists, etc. quite well, so I think it's appropriate to use the name zip for it. For other notions of zipping, we already have Applicative functors, and could potentially add Naperian functors for a sort of "guaranteed full zip" where the shape of both arguments and the result is always the same.

Thoughts on this? I'm especially unsure about it because the FunctorFilter/MonadCombine situation seems to be changing.

@codecov-io
Copy link

codecov-io commented Jul 2, 2017

Codecov Report

Merging #1755 into master will increase coverage by 0.08%.
The diff coverage is 94.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1755      +/-   ##
==========================================
+ Coverage    94.2%   94.28%   +0.08%     
==========================================
  Files         256      260       +4     
  Lines        4207     4255      +48     
  Branches       84       85       +1     
==========================================
+ Hits         3963     4012      +49     
+ Misses        244      243       -1
Impacted Files Coverage Δ
core/src/main/scala/cats/syntax/align.scala 100% <100%> (ø)
...c/main/scala/cats/laws/discipline/AlignTests.scala 100% <100%> (ø)
core/src/main/scala/cats/instances/map.scala 95.12% <100%> (+0.67%) ⬆️
core/src/main/scala/cats/instances/stream.scala 95.08% <100%> (+0.43%) ⬆️
core/src/main/scala/cats/instances/vector.scala 97.95% <100%> (+0.18%) ⬆️
core/src/main/scala/cats/instances/list.scala 100% <100%> (ø) ⬆️
laws/src/main/scala/cats/laws/AlignLaws.scala 100% <100%> (ø)
core/src/main/scala/cats/instances/option.scala 100% <100%> (ø) ⬆️
core/src/main/scala/cats/Align.scala 40% <40%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c012417...e343345. Read the comment docs.

@ceedubs
Copy link
Contributor

ceedubs commented Dec 10, 2017

Thanks @julianmichael! I personally really like the Align type class. This is a great PR; sorry that I missed it when it was submitted.

Do you have any thoughts on what the implications are on Align now that cats-mtl has been extracted into its own library and FunctorFilter etc have been moved to it?

@julianmichael
Copy link
Author

No problem at all @ceedubs. My main motivation for this PR was I wanted easy zipping and this seemed like the first step. So I'm thinking about how to do this so it'd fit well with a Zip typeclass as well. My conclusions:

  1. A version of Align without nil would be nice actually, since notions of aligning and zipping make sense for nonempty containers like NonEmptyList. In this case we can relate align to zip with the law

    zip(fa, fa).map(Function.tupled(Ior.both(_, _))) === align(fa, fa)

    which seems weak, but combined with the laws for align and zip independently, probably goes pretty far.

  2. A version with nil would also be useful, even without FunctorEmpty. It gives us some more guarantees/laws, it gives us a couple of nice MonoidKs via

    def empty[A] = nil[A]
    def combineK[A, B](fa, fb) = fa.align(fb).map(_.left) // or _.right

    and it nicely characterizes the monoidal structure of Kleisli[Option, I, ?] where I is some indexing type. (If we add representable functors, this connection would be even stronger, since we can give an Align instance to any OptionT[F, ?] where F is representable). However, it does result in an extra empty-like value floating around in addition to MonoidK's. That could maybe be resolved by making Align extend MonoidK and using the implementation above, but... that seems non-ideal.

  3. A version with nil that extends FunctorEmpty would be even better since it tightens the connection to Kleisli[Option, I, ?] and gives us a default implementation of zip in terms of align with the stronger law:

    zip(fa, fb) === align(fa, fb).collect { case Ior.Both(a, b) => (a, b) }

    But it's probably not a huge deal, because—assuming we make Align extend a Zip typeclass—given that everything I can think of that can be aligned can also be zipped, we could just ask instances of Align to implement both. And that would need to be done regardless in the nonempty case.

I'm kind of biased toward bringing FunctorEmpty back into cats regardless, just because I think it's nice to be able to abstract over collection types outside of MTL-style code, and I feel like Traverse is great and provides everything except collect/filter.

Regardless of that, I think it could make sense to have two versions of Align, one with and one without nil. It's kind of a proliferation of typeclasses but maybe cats-more would be an appropriate place for that as suggested in #1940.

OTOH, if including Aligns, Zip, Representable etc. (and maybe more, like the other classes in Haskell's Data.Align) seems unnecessary for cats-core, and cats-more doesn't pan out, it might make more sense to leave it to a third-party lib. No guarantees I would be the one to do that though... :)

@ceedubs
Copy link
Contributor

ceedubs commented Dec 19, 2017

@julianmichael Thanks for the detailed thoughts!

I agree with you that a version without nil would be really handy for structures like NonEmptyList.

I'm also a little partial to FunctorFilter/TraverseFilter, but it is true that diamond patterns in type class hierarchies lead to issues with the type class encoding that Cats uses. I wonder if this is an issue that will affect Align once you have both empty and non-empty versions of it. Maybe cats-mtl is a good home for this? I'm not sure. I'm curious what @adelbertc and @kailuowang think.

@kailuowang
Copy link
Contributor

Sorry I dropped the ball on this one. Let's see if we can make a final decision on FunctorEmpty and TraverseEmpty in #2348 and it would make a decision here easier. For now, I am just going to schedule it to be 1.3, which will probably prevent it from falling off the crack again.

@kailuowang kailuowang added this to the 1.3 milestone Aug 14, 2018
@kailuowang
Copy link
Contributor

I think those two type classes are coming back to core #2405, once merged, this would be unblocked. We are probably going to rush for a 1.3 release. @julianmichael let me know if you want to try squeeze it in or take your time and aim for the next 1.4 release.

@julianmichael
Copy link
Author

Cool! Would rather not rush it. We seem to have a lot of new stuff in addition to FunctorEmpty and TraverseEmpty, including Representable, some uses of compositional typeclass encoding, and more non-empty and unordered typeclasses and datatypes. I'd like to play with it a bit to make sure they all work nicely together.

@kailuowang kailuowang modified the milestones: 1.3, 1.4 Aug 15, 2018
@kailuowang kailuowang modified the milestones: 1.4, 1.5 Sep 12, 2018
@kailuowang kailuowang modified the milestones: 1.5-RC0, 1.6 Nov 16, 2018
@kailuowang kailuowang modified the milestones: 1.6, 1.7 Jan 8, 2019
@kailuowang kailuowang removed this from the 1.7 milestone Feb 27, 2019
@kailuowang
Copy link
Contributor

Closing stale PRs. Feel free to reopen if there is interest to revive the effort.

@kailuowang kailuowang closed this Aug 5, 2019
@LukaJCB LukaJCB mentioned this pull request Sep 20, 2019
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.

4 participants