Add `SortedMap` and `SortedSet` instances/Move `Set` and `Map` instances to Alleycats #1972

LukaJCB · 2017-10-16T03:01:03Z

Should fix #1966

kailuowang · 2017-10-16T03:05:09Z

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

+        else fb.map(fb => map2(fa, fb)(f))
+
+      override def ap[A, B](ff: SortedMap[K, A => B])(fa: SortedMap[K, A]): SortedMap[K, B] =
+        ??? //fa.flatMap { case (k, a) => ff.get(k).map(f => (k, f(a))) }(scala.collection.breakOut)


still WIP? same below

Yes, sorry. I meant to state so in the description 😅

johnynek

added #1973 about my doubts.

johnynek · 2017-10-16T05:11:59Z

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

+    var a, b, n = 0
+    var c = 1;
+    x foreach { case (k, v) =>
+      // use the default hash on keys because that's what Scala's Map does


I don't think we want to do that here actually. Map[K, V] needs to do this because the hash in question in a Map[K, V] is the .hashCode. But I think we can be unconstrained on a SortedMap since it is only using our Order typeclass.

johnynek · 2017-10-16T05:15:07Z

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

+        fa.foldLeft(b) { case (x, (k, a)) => f(x, a)}
+
+      def foldRight[A, B](fa: SortedMap[K, A], lb: Eval[B])(f: (A, Eval[B]) => Eval[B]): Eval[B] =
+        Foldable.iterateRight(fa.values.iterator, lb)(f)


I thought this was actually unsafe since it captures a mutable iterator? I think to make it safe, it needs to have () => Iterator[A] which it can call to get a new one each time (which is another way of saying the Iterable[A] and it calls .iterator internally.

We had some bug related to this in the past. Not sure why we didn't fix Foldable.iterateRight. Actually, it was related, but not exactly the same. See #1716.

codecov-io · 2017-10-16T14:54:23Z

Codecov Report

Merging #1972 into master will decrease coverage by 0.24%.
The diff coverage is 86.52%.

@@            Coverage Diff             @@
##           master    #1972      +/-   ##
==========================================
- Coverage   95.49%   95.24%   -0.25%     
==========================================
  Files         298      301       +3     
  Lines        4814     4922     +108     
  Branches      120      123       +3     
==========================================
+ Hits         4597     4688      +91     
- Misses        217      234      +17

Impacted Files	Coverage Δ
...s-core/src/main/scala/alleycats/std/iterable.scala	`100% <ø> (ø)`	⬆️
...in/scala/cats/laws/discipline/ReducibleTests.scala	`100% <ø> (ø)`	⬆️
...eycats-core/src/main/scala/alleycats/std/set.scala	`100% <ø> (+50%)`	⬆️
core/src/main/scala/cats/Foldable.scala	`100% <ø> (ø)`	⬆️
laws/src/main/scala/cats/laws/TraverseLaws.scala	`92.3% <100%> (+1.98%)`	⬆️
core/src/main/scala/cats/instances/map.scala	`90% <100%> (-4.29%)`	⬇️
laws/src/main/scala/cats/laws/FoldableLaws.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/instances/set.scala	`100% <100%> (ø)`	⬆️
...ain/scala/cats/laws/discipline/TraverseTests.scala	`100% <100%> (ø)`	⬆️
...rc/main/scala/cats/laws/discipline/Arbitrary.scala	`92.95% <100%> (+0.77%)`	⬆️
... and 9 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 c4ee747...2aef355. Read the comment docs.

LukaJCB · 2017-10-16T16:14:02Z

Damn there's no mutable SortedMap pre-Scala 2.12

LukaJCB · 2017-10-16T23:07:21Z

So the tests fail for 2.11 and earlier for some reason. Will have to investigate

…into LukaJCB-add-sorted-instances

kailuowang · 2017-10-26T20:59:08Z

@LukaJCB I submitted a PR against your branch LukaJCB#2
let me know if that sounds a good idea to you.

kailuowang · 2017-10-27T18:33:07Z

tests/src/test/scala/cats/tests/FoldableSuite.scala

@@ -218,14 +218,6 @@ class FoldableSuiteAdditional extends CatsSuite {
    checkFoldMStackSafety[Vector](_.toVector)
  }

-  test("Foldable[Set].foldM stack safety") {
-    checkFoldMStackSafety[Set](_.toSet)
-  }


shall we replace these two foldMStackSafety tests with SortedSet and SortedMap ones?

kailuowang · 2017-10-27T18:35:44Z

tests/src/test/scala/cats/tests/ParallelTests.scala

@@ -30,7 +30,7 @@ class ParallelTests extends CatsSuite with ApplicativeErrorForEitherTest {
  }

  test("ParTraverse_ identity should be equivalent to parSequence_") {
-    forAll { es: Set[Either[String, Int]] =>
+    forAll { es: List[Either[String, Int]] =>


why not use SortedSet? (Either has Order instance)

kailuowang · 2017-10-27T18:39:26Z

tests/src/test/scala/cats/tests/RegressionSuite.scala

-    type StringMap[A] = Map[String, A]
-    val intMap: StringMap[Int] = Map("one" -> 1, "two" -> 2, "six" -> 6, "eight" -> 8)
-    intMap.traverse(validate) should === (Either.left("6 is greater than 5"))
-    checkAndResetCount(3)


Not sure what this regression test is about, do you know? If neither of us are sure maybe we should replace it with a sortedMap one?

Apparently it's about #513. I had to adjust this test slightly, because the order of the Strings matter now, so it wouldn't work if we just change the type of the collection :)

LukaJCB · 2017-10-28T08:00:00Z

Everything should be addressed now 🎉 Thanks everyone!

johnynek · 2017-10-28T18:29:53Z

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

-  implicit def catsStdInstancesForMap[K]: Traverse[Map[K, ?]] with FlatMap[Map[K, ?]] =
-    new Traverse[Map[K, ?]] with FlatMap[Map[K, ?]] {
-
-      def traverse[G[_], A, B](fa: Map[K, A])(f: A => G[B])(implicit G: Applicative[G]): G[Map[K, B]] = {


Can we add some law that this violates? I hate to remove it if we don’t actually have a law that it violates. I think we can write some (the original ticket had an example). I fear we could have other lawless instances without a law.

johnynek · 2017-10-28T18:31:20Z

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


      def empty[A]: Set[A] = Set.empty[A]

      def combineK[A](x: Set[A], y: Set[A]): Set[A] = x | y

-      def foldLeft[A, B](fa: Set[A], b: B)(f: (B, A) => B): B =


Same concern here about adding a law that this violates. Maybe a == b implies toList(a) == toList(b)

We're going to need to spend some more time to find something. That one won't work, because that way e.g
Either.Left(123) =!= Either.Left(-30) would imply List() =!= List()

I'm not sure if it's the best solution but I worked around it, by only considering the positive case. I.e a == b implies toList(a) == toList(b) but not a != b implies toList(a) != toList(b)

This is what I meant (which is why I didn’t say (a == b) == (a.toList == b.toList))

johnynek · 2017-10-29T18:10:04Z

This looks good to me. One question: do you know if this test would actually have caught Map/Set problems? I guess it must have since small maps/sets are just tuples and created in the order added.

LukaJCB · 2017-10-29T18:52:10Z

Yes I tested these on master and they failed for set/map.

LukaJCB · 2017-10-29T19:02:34Z

Although to be fair, I ran into some problems because it might not actually generate a situation where the tests fail every time because it's rare that they generate two different collections with the same values but different orderings.

realized that the Foldable test could be improved.

kailuowang · 2017-10-30T15:09:02Z

I couldn't think of a way to write the law so that will consistently fail on unordered data types either. Another option we have is to simply remove the orderedConsistency test and simply document the fact that Foldable only supports ordered F[_], for unordered F[_] use the new UnorderedFoldable and UnorderedTraverse, which we should probably do anyway.

kailuowang · 2017-10-30T20:16:45Z

thanks so much for pushing this through

stew added the in progress label Oct 16, 2017

kailuowang reviewed Oct 16, 2017

View reviewed changes

LukaJCB force-pushed the add-sorted-instances branch from df28b39 to 24fb152 Compare October 16, 2017 03:09

johnynek reviewed Oct 16, 2017

View reviewed changes

LukaJCB force-pushed the add-sorted-instances branch from 24fb152 to 5ec40c9 Compare October 16, 2017 13:12

Add SortedMap instances

a1705a9

LukaJCB force-pushed the add-sorted-instances branch from 5ec40c9 to a1705a9 Compare October 16, 2017 14:08

Add law tests

2ec0b0a

Merge branch 'master' into add-sorted-instances

fef41c1

LukaJCB force-pushed the add-sorted-instances branch from 035461b to c226af6 Compare October 16, 2017 17:53

kailuowang mentioned this pull request Oct 16, 2017

Getting ready for RC1 #1974

Merged

12 tasks

LukaJCB force-pushed the add-sorted-instances branch from c226af6 to 26c1d9a Compare October 16, 2017 21:32

LukaJCB force-pushed the add-sorted-instances branch from 26c1d9a to 9c76d12 Compare October 17, 2017 13:50

Remove optimized combineAll

62ec8d4

LukaJCB force-pushed the add-sorted-instances branch from 9c76d12 to 62ec8d4 Compare October 17, 2017 14:37

kailuowang mentioned this pull request Oct 17, 2017

Foldable.toList potentially inconsistent for Map #1831

Closed

Luka Jacobowitz added 2 commits October 18, 2017 19:34

Add SortedSet instances

ec82e5f

Try to fix some tests

1bf9b94

LukaJCB mentioned this pull request Oct 19, 2017

Add UnorderedFoldable and UnorderedTraverse #1981

Merged

Merge branch 'master' into add-sorted-instances

8ff298e

LukaJCB force-pushed the add-sorted-instances branch 2 times, most recently from 58e6907 to fb37bba Compare October 24, 2017 16:44

Rename

a5948aa

LukaJCB force-pushed the add-sorted-instances branch from fb37bba to a5948aa Compare October 24, 2017 16:50

kailuowang added 2 commits October 26, 2017 16:07

Merge branch 'add-sorted-instances' of https://github.com/LukaJCB/cats …

32c9aa2

…into LukaJCB-add-sorted-instances

wip move map and set instance to alleycats

4e13e8e

kailuowang reviewed Oct 27, 2017

View reviewed changes

Luka Jacobowitz added 2 commits October 28, 2017 09:47

Fix tut docs

40f9de5

Address feedback

36eeada

LukaJCB added the Source Breaking label Oct 28, 2017

LukaJCB requested review from johnynek and kailuowang October 28, 2017 14:21

LukaJCB removed the in progress label Oct 28, 2017

johnynek reviewed Oct 28, 2017

View reviewed changes

Luka Jacobowitz added 2 commits October 29, 2017 16:46

Add order consistency test

4e6456a

Add law for foldable

8745b55

LukaJCB force-pushed the add-sorted-instances branch from 59a760a to 8745b55 Compare October 29, 2017 17:22

Remove failing tests from Alleycats

37e36ea

LukaJCB force-pushed the add-sorted-instances branch from e32a461 to 37e36ea Compare October 30, 2017 09:14

kailuowang previously approved these changes Oct 30, 2017

View reviewed changes

kailuowang changed the title ~~Add SortedMap instances~~ Add SortedMap and SortedSet instances Oct 30, 2017

Add requirement that Foldable instances should be ordered

2aef355

kailuowang changed the title ~~Add SortedMap and SortedSet instances~~ Add SortedMap and SortedSet instances/Move Set and Map instances to Alleycats Oct 30, 2017

johnynek approved these changes Oct 30, 2017

View reviewed changes

kailuowang merged commit 1f0cba0 into typelevel:master Oct 30, 2017

kailuowang added this to the 1.0.0-RC1 milestone Oct 31, 2017

kailuowang added the enhancement label Dec 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `SortedMap` and `SortedSet` instances/Move `Set` and `Map` instances to Alleycats #1972

Add `SortedMap` and `SortedSet` instances/Move `Set` and `Map` instances to Alleycats #1972

LukaJCB commented Oct 16, 2017

kailuowang Oct 16, 2017 •

edited

Loading

LukaJCB Oct 16, 2017

johnynek left a comment

johnynek Oct 16, 2017

johnynek Oct 16, 2017

codecov-io commented Oct 16, 2017 •

edited

Loading

LukaJCB commented Oct 16, 2017

LukaJCB commented Oct 16, 2017

kailuowang commented Oct 26, 2017

kailuowang Oct 27, 2017

kailuowang Oct 27, 2017

kailuowang Oct 27, 2017

LukaJCB Oct 28, 2017

LukaJCB commented Oct 28, 2017

johnynek Oct 28, 2017

LukaJCB Oct 29, 2017

johnynek Oct 28, 2017

LukaJCB Oct 29, 2017

LukaJCB Oct 29, 2017

johnynek Oct 29, 2017

johnynek commented Oct 29, 2017

LukaJCB commented Oct 29, 2017

LukaJCB commented Oct 29, 2017

kailuowang commented Oct 30, 2017

kailuowang commented Oct 30, 2017

Add SortedMap and SortedSet instances/Move Set and Map instances to Alleycats #1972

Add SortedMap and SortedSet instances/Move Set and Map instances to Alleycats #1972

Conversation

LukaJCB commented Oct 16, 2017

kailuowang Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 16, 2017 • edited Loading

Codecov Report

LukaJCB commented Oct 16, 2017

LukaJCB commented Oct 16, 2017

kailuowang commented Oct 26, 2017

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

LukaJCB commented Oct 28, 2017

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

johnynek commented Oct 29, 2017

LukaJCB commented Oct 29, 2017

LukaJCB commented Oct 29, 2017

kailuowang commented Oct 30, 2017

kailuowang commented Oct 30, 2017

Add `SortedMap` and `SortedSet` instances/Move `Set` and `Map` instances to Alleycats #1972

Add `SortedMap` and `SortedSet` instances/Move `Set` and `Map` instances to Alleycats #1972

kailuowang Oct 16, 2017 •

edited

Loading

codecov-io commented Oct 16, 2017 •

edited

Loading