Adding get for Foldable #1464

yilinwei · 2016-11-13T19:16:18Z

No description provided.

codecov-io · 2016-11-13T23:10:51Z

Codecov Report

Merging #1464 into master will increase coverage by 0.55%.
The diff coverage is 97.56%.

@@            Coverage Diff             @@
##           master    #1464      +/-   ##
==========================================
+ Coverage   93.38%   93.94%   +0.55%     
==========================================
  Files         241      241              
  Lines        4054     4096      +42     
  Branches      141      156      +15     
==========================================
+ Hits         3786     3848      +62     
+ Misses        268      248      -20

Impacted Files	Coverage Δ
core/src/main/scala/cats/Reducible.scala	`93.22% <0%> (-1.61%)`	⬇️
core/src/main/scala/cats/instances/set.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/Foldable.scala	`98.55% <100%> (+4.89%)`	⬆️
core/src/main/scala/cats/data/Ior.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/instances/either.scala	`100% <100%> (+2.38%)`	⬆️
core/src/main/scala/cats/data/NonEmptyVector.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/IdT.scala	`70.83% <100%> (+2.65%)`	⬆️
core/src/main/scala/cats/data/Validated.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/instances/vector.scala	`97.77% <100%> (+0.05%)`	⬆️
core/src/main/scala/cats/instances/try.scala	`96.36% <100%> (+0.06%)`	⬆️
... and 32 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 586ea08...71d4fd6. Read the comment docs.

edmundnoble · 2016-11-14T03:49:05Z

core/src/main/scala/cats/Foldable.scala

+    * The default implementation of this is based on `foldLeft`, and thus will
+    * always fold across the entire structure.
+    */
+  def get[A](fa: F[A])(idx: Long): Option[A] =


Has it not been investigated, whether it's possible to do this with foldRight?

If we are going to add this, I think we need to implement the faster thing for Vector, NonEmpty*, List, etc...

@edmundnoble I don't think it's possible to do it with foldRight unless we choose to have a mutable index outside of the loop, since the the foldRight only 'pulls' A rather than the accumulation.

I've got no personal objections to doing it with a mutable var, but what are your thoughts?

For the default implementation I think .foldLeft is probably the best.

If you built a custom monadic data type with the right semantics I think you could use .foldM to implement .get with short-circuiting. But I'm not sure it's worth doing compared to just letting data types override the method if desired.

The implementation now uses foldM over Either, looks good to me. 👍

johnynek · 2016-11-14T17:56:34Z

core/src/main/scala/cats/Foldable.scala

+  /**
+    * Get the element at the index of the `Foldable`
+    */
+  def apply[A](fa: F[A])(idx: Long): A = get(fa)(idx).get


Not crazy about adding methods like this without an unsafe warning. Something like getUnsafe? (I can't recall what our unsafe convention is: prefix or suffix).

I'm not quite sure that I agree with this, since the apply/get method is named the same as the scala collection methods. I quite like the behavior that in the particular case that we have a concrete type it would defer to that behavior.

apply is "special" because it is an unsafe method - I would prefer to have any/all unsafe methods in Cats tagged with some indication that it's unsafe in the name. Or better yet just not have it, and users can call .get on get if they want to be unsafe.

I would agree for functions which aren't obviously unsafe - the unsafeRun in the Stream in fs2 comes to mind. However, for getting any index or key in a Map or List using apply in scala is already implied shorthand for get.get?

Since I would argue that users should already be familiar with this behaviour, I can't see much benefit in calling it getUnsafe.

I don't feel particularly strongly enough to not change it, but I don't think to signal it's an unsafe method to users is a good enough reason.

Just to be clear - I'll change the name since any change to this ought to change the rest of the names for the sake of consistency but I thought it would be good to raise these points.

@adelbertc I do not pay enough attention to have a definitive list. An obvious example would be the fact that all monads are stacksafe:

https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/FlatMap.scala#L102

@jedws Ah there was quite a bit of talk on the location of that particular function during this past weekend at Scala by the Bay :-)

I'd like to avoid having such a discussion on this particular ticket, but if it's OK with you perhaps we can chat somewhere else about this? I'd be very interested in hearing your thoughts about it. I could ping you on IRC, or if you're on Gitter that works too.

@jedws I agree we should not use this ticket to rehash old discussions, but I would be interested to hear from you if you can give examples of monads that can't be made stack safe.

Going back to this line in particular, I would prefer not having this method, especially on something as abstract as Foldable.

@adelbertc I presume that you mean regardless of the name? Should getUnsafe be removed from corresponding classes for the sake of consistency?

johnynek · 2016-11-14T17:57:06Z

core/src/main/scala/cats/instances/vector.scala

@@ -73,6 +73,8 @@ trait VectorInstances extends cats.kernel.instances.VectorInstances {

      override def size[A](fa: Vector[A]): Long = fa.size.toLong

+      override def get[A](fa: Vector[A])(idx: Long): Option[A] = if (fa.size > idx && idx > 0) Some(fa(idx.toInt)) else None


Ahh, I see you have the Vector here, but not yet List (which can be made faster than a full fold) or NonEmpties.

Added for List and NonEmpties

johnynek · 2016-11-15T01:02:22Z

core/src/main/scala/cats/instances/list.scala

+      override def get[A](fa: List[A])(idx: Long): Option[A] =
+        if (idx < 0 || fa.isEmpty) {
+          None
+        } else if (idx < 0) {


this should be idx > 0 right?

Yes! Will change this.

johnynek · 2016-11-15T01:05:16Z

core/src/main/scala/cats/data/NonEmptyVector.scala

@@ -216,6 +216,9 @@ private[data] sealed trait NonEmptyVectorInstances {
      override def foldRight[A, B](fa: NonEmptyVector[A], lb: Eval[B])(f: (A, Eval[B]) => Eval[B]): Eval[B] =
        fa.foldRight(lb)(f)

+      override def get[A](fa: NonEmptyVector[A])(idx: Long): Option[A] =
+        fa.get(idx.toInt)


we might want to check that idx fits in a Int else return None, right?

In general I wonder if it's worth just using Int as the index. I can't think of many structures you can index into with Long values that you'd also want to fold over. (But that doesn't mean they don't exist!)

yeah, I think Int is more consistent with scala to be sure. Also, it is going to take quite a while to index past the 2 billionth element. If you need that, I doubt you are hindered by not having it in the typeclass directly.

I used a Long because the size is a Long. It seems strange that your collection can be a certain size without allowing you to get elements close to the end.

I don't think it makes sense to offer size as a long and get with an idx: Int. 👍 to checking if it fits if an Int and returning None otherwise.

johnynek · 2016-11-17T23:34:53Z

core/src/main/scala/cats/instances/list.scala

@@ -70,6 +70,16 @@ trait ListInstances extends cats.kernel.instances.ListInstances {
          G.map2Eval(f(a), lglb)(_ :: _)
        }.value

+      @tailrec
+      override def get[A](fa: List[A])(idx: Long): Option[A] =


can I request we avoid .head etc... unless needed? Like:

fa match { case Nil => None case h :: tail => if (idx < 0) None else if (idx == 0) h else get(tail)(idx - 1) }

johnynek · 2016-11-17T23:40:46Z

core/src/main/scala/cats/instances/vector.scala

@@ -73,6 +73,8 @@ trait VectorInstances extends cats.kernel.instances.VectorInstances {

      override def size[A](fa: Vector[A]): Long = fa.size.toLong

+      override def get[A](fa: Vector[A])(idx: Long): Option[A] = if (idx < Int.MaxValue && fa.size > idx && idx >= 0) Some(fa(idx.toInt)) else None


can you put a \n after = to put this code under def get?

edmundnoble · 2017-05-12T16:41:25Z

@yilinwei Do you have time to address the code review comments? This is very useful.

yilinwei · 2017-05-12T19:07:31Z

@edmundnoble Yes; I'll update/rebase this tomorrow.

edmundnoble · 2017-05-14T19:52:51Z

Hey, sorry for dragging this out; could you also override get for all other Foldable/Traverse instances? Option, Either, Try, Id, IdT, Set, Map, EitherKTraverse, OneAnd, Validated, Const, Ior, Stream, TupleN, ComposedTraverse are all the ones I can find.

yilinwei · 2017-05-14T23:02:53Z

@edmundnoble I've done all except for EitherKTraverse and ComposedTraverse because I'd pretty much write the same thing as the current default implementation (minus maybe one call).

Do you have a different implementation in mind?

edmundnoble · 2017-05-14T23:11:23Z

@yilinwei I thought about it some more, and ComposedTraverse is hard to override efficiently because to avoid making extra passes you have to get on the inner G and get the size of the G at once. EitherKTraverse's though looks like:

  override def get[A](fa: EitherK[F, G, A])(idx: Long): Option[A] =
    fa.run.fold(F.get(_)(idx), G.get(_)(idx))

yilinwei · 2017-05-14T23:16:50Z

@edmundnoble I arrived at the same conclusion for Composed but I'm ashamed to say I temporarily forgot how subclassing worked for EitherK...

ceedubs · 2017-05-18T01:51:41Z

The 2.10 build had timed out, but after a restart it worked fine. I suspect that it won't be an issue with split out of the JS builds that has recently happened on master.

👍 from me. Thanks @yilinwei!

peterneyens

Sorry for the very late feedback.

I left a few comments, but this looks really good. Thanks also for the added tests and cleanup in FoldableTests. 👍

peterneyens · 2017-05-19T09:23:21Z

core/src/main/scala/cats/data/OneAnd.scala

@@ -117,6 +117,9 @@ private[data] sealed trait OneAndInstances extends OneAndLowPriority2 {
    new NonEmptyReducible[OneAnd[F, ?], F] {
      override def split[A](fa: OneAnd[F, A]): (A, F[A]) = (fa.head, fa.tail)

+      override def get[A](fa: OneAnd[F, A])(idx: Long): Option[A] =
+        if (idx == 0L) Some(fa.head) else F.get(fa.tail)(idx - 1L)


I think we can add this implementation to NonEmptyReducible (in Reducible.scala) as well,

Done. I know there isn't a test currently; but I think that if such a test exists it should be comparing the default implementation to any overrides which exist.

Do you mind if we put that on a different issue because I think that the consistency of the methods is important.

peterneyens · 2017-05-19T09:23:25Z

core/src/main/scala/cats/data/Validated.scala

@@ -404,6 +404,9 @@ private[data] sealed abstract class ValidatedInstances2 {
      override def size[A](fa: Validated[E, A]): Long =
        fa.fold(_ => 0L, _ => 1L)

+      override def get[A](fa: Validated[E, A])(idx: Long): Option[A] =
+        if (idx == 0L) fa.fold(_ => None, Some(_)) else None


We can use Validated#toOption.

peterneyens · 2017-05-19T09:40:01Z

core/src/main/scala/cats/instances/try.scala

+          case Failure(_) => None
+          case Success(a) =>
+            if (idx == 0L) Some(a) else None
+        }


Nitpicking: maybe like in Validated :

if (idx == 0L) fa.toOption else None

peterneyens · 2017-05-19T09:54:08Z

tests/src/test/scala/cats/tests/FoldableTests.scala

+  def iterator[T](tuple: (Int, T)) = Some(tuple._2).iterator
+}
+
+class FoldableOneAndCheck extends FoldableCheck[OneAnd[List, ?]]("oneAnd") {


This is sort of already checked through ReducibleNonEmptyStreamCheck in OneAndTests.scala

I agree there is some duplication, but I don't think it should be tackled in this issue and should probably a different issue where we discuss what to do with tests for a typeclass which aren't part of the discipline laws.

peterneyens · 2017-05-19T09:56:03Z

tests/src/test/scala/cats/tests/FoldableTests.scala

+
+import FoldableComposedCheck._
+
+class FoldableComposedCheck extends FoldableCheck[λ[α => List[Option[α]]]]("composed") {


We can maybe check this with Nested[List, Option, ?], that way we don't need the FoldableComposedCheck companion object. WDYT?

👍 I thought this existed but couldn't remember the name of it.

peterneyens · 2017-05-19T09:58:29Z

core/src/main/scala/cats/data/Ior.scala

+    override def size[B](fa: A Ior B): Long = fa.fold(_ => 0L, _ => 1L, (_, _) => 1L)
+
+    override def get[B](fa: A Ior B)(idx: Long): Option[B] =
+      if (idx == 0L) fa.fold(_ => None, Some(_), (_, b) => Some(b)) else None


We can use Ior#toOption instead of the fold.

peterneyens

Thanks for the changes.

peterneyens · 2017-05-19T12:45:14Z

Merging with 3 👍, thanks @yilinwei.

ceedubs · 2017-05-19T12:28:40Z

core/src/main/scala/cats/Reducible.scala

@@ -226,6 +226,9 @@ abstract class NonEmptyReducible[F[_], G[_]](implicit G: Foldable[G]) extends Re
    1 + G.size(tail)
  }

+  override def get[A](fa: F[A])(idx: Long): Option[A] =
+    if (idx == 0L) Some(split(fa)._1) else G.get(split(fa)._2)(idx)


Shouldn't this be else G.get(split(fa)._2)(idx - 1L)?

It's a bit funny that the only line of your PR that isn't covered by a unit test is the one with a bug in it 😜

😢 You're right.

Shit, looked over that one too quickly as well. Thanks @ceedubs for paying attention.

ceedubs · 2017-05-19T13:00:23Z

@peterneyens @yilinwei oops I forgot to hit the "submit review" button after making this review comment earlier :(

#1464 (comment)

yilinwei force-pushed the foldable-apply branch 7 times, most recently from e77602d to cc8e7c9 Compare November 13, 2016 22:24

edmundnoble reviewed Nov 14, 2016

View reviewed changes

johnynek reviewed Nov 14, 2016

View reviewed changes

yilinwei force-pushed the foldable-apply branch from cc8e7c9 to 015b927 Compare November 14, 2016 23:40

johnynek reviewed Nov 15, 2016

View reviewed changes

yilinwei force-pushed the foldable-apply branch from 015b927 to 165da9b Compare November 15, 2016 21:06

johnynek reviewed Nov 17, 2016

View reviewed changes

kailuowang added this to the 1.0.0-MF milestone May 12, 2017

yilinwei force-pushed the foldable-apply branch 2 times, most recently from 1863379 to b500bf1 Compare May 13, 2017 22:52

adding get to Foldable

737e5de

yilinwei force-pushed the foldable-apply branch from b500bf1 to 737e5de Compare May 13, 2017 23:03

overriding get for Foldable instances

d94f5e8

edmundnoble approved these changes May 15, 2017

View reviewed changes

Adding size and get for EitherK

c829136

yilinwei force-pushed the foldable-apply branch from 40426c4 to c829136 Compare May 15, 2017 09:10

yilinwei added 2 commits May 15, 2017 11:14

add missing tests and bugfix existing instance

6dcea43

adding FoldableComposedCheck

f093fb1

ceedubs approved these changes May 18, 2017

View reviewed changes

peterneyens reviewed May 19, 2017

View reviewed changes

peterneyens changed the title ~~Adding apply and get for Foldable~~ Adding get for Foldable May 19, 2017

Addressing review comments

71d4fd6

peterneyens approved these changes May 19, 2017

View reviewed changes

peterneyens merged commit cc4080c into typelevel:master May 19, 2017

ceedubs reviewed May 19, 2017

View reviewed changes

		@@ -73,6 +73,8 @@ trait VectorInstances extends cats.kernel.instances.VectorInstances {

		override def size[A](fa: Vector[A]): Long = fa.size.toLong

		override def get[A](fa: Vector[A])(idx: Long): Option[A] = if (fa.size > idx && idx > 0) Some(fa(idx.toInt)) else None


		import FoldableComposedCheck._

		class FoldableComposedCheck extends FoldableCheck[λ[α => List[Option[α]]]]("composed") {

Adding get for Foldable #1464

Adding get for Foldable #1464

Conversation

yilinwei commented Nov 13, 2016

codecov-io commented Nov 13, 2016 • edited Loading

Codecov Report

edmundnoble Nov 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yilinwei Nov 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmundnoble commented May 12, 2017

yilinwei commented May 12, 2017

edmundnoble commented May 14, 2017 • edited Loading

yilinwei commented May 14, 2017 • edited Loading

edmundnoble commented May 14, 2017

yilinwei commented May 14, 2017

ceedubs commented May 18, 2017

peterneyens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterneyens left a comment

Choose a reason for hiding this comment

peterneyens commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceedubs commented May 19, 2017 • edited Loading

codecov-io commented Nov 13, 2016 •

edited

Loading

edmundnoble Nov 14, 2016 •

edited

Loading

yilinwei Nov 14, 2016 •

edited

Loading

edmundnoble commented May 14, 2017 •

edited

Loading

yilinwei commented May 14, 2017 •

edited

Loading

ceedubs commented May 19, 2017 •

edited

Loading