Add UnorderedFoldable and UnorderedTraverse #1981

LukaJCB · 2017-10-19T18:11:28Z

As discussed in #1927, together with #1972 and #1966 this should fix #1831.
In this PR, I remove the Traverse and Foldable instances from Map and Set and replace them with UnorderedTraverse and UnorderedFoldable. I think we could let Foldable and Traverse extend these.

This PR depends on #1927.

…mmutativeSemigroup, ?]

This reverts commit 0cd4eb9.

…ons there

…l functions there" This reverts commit d308cf7.

This reverts commit 0ffbc55.

codecov-io · 2017-10-19T19:24:57Z

Codecov Report

Merging #1981 into master will decrease coverage by 0.1%.
The diff coverage is 90.81%.

@@            Coverage Diff             @@
##           master    #1981      +/-   ##
==========================================
- Coverage   95.25%   95.14%   -0.11%     
==========================================
  Files         305      311       +6     
  Lines        5179     5252      +73     
  Branches      127      131       +4     
==========================================
+ Hits         4933     4997      +64     
- Misses        246      255       +9

Impacted Files	Coverage Δ
...a/cats/laws/discipline/NonEmptyTraverseTests.scala	`100% <ø> (ø)`	⬆️
laws/src/main/scala/cats/laws/TraverseLaws.scala	`92.3% <ø> (ø)`	⬆️
core/src/main/scala/cats/syntax/foldable.scala	`100% <ø> (ø)`	⬆️
laws/src/main/scala/cats/laws/FoldableLaws.scala	`100% <ø> (ø)`	⬆️
...in/scala/cats/laws/discipline/ReducibleTests.scala	`100% <ø> (ø)`	⬆️
...ain/scala/cats/laws/discipline/TraverseTests.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/instances/set.scala	`100% <100%> (ø)`	⬆️
.../cats/laws/discipline/UnorderedTraverseTests.scala	`100% <100%> (ø)`
core/src/main/scala/cats/Foldable.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/instances/map.scala	`92.59% <100%> (+2.59%)`	⬆️
... and 13 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 9020258...ea38cd6. Read the comment docs.

kailuowang · 2017-10-20T14:35:32Z

I like the idea of introducing these two new type classes.
I am not sure they should be introduced to the Traverse/Foldable hierarchy. It's not clear to me that for Traverse and Foldable and their instances, what benefit them for inheriting UnorderedFoldable and UnorderedTraverse. They have to override most methods and they got two additional unorderred methods that are not needed by their instances. Is there a concrete example in which such inheritance arrangement would be beneficial?

LukaJCB · 2017-10-20T14:46:39Z

Good question! I don't think there's a huge benefit apart from deduplication.

kailuowang · 2017-10-20T15:34:19Z

You probably guessed what I would say next. They are useful but

not sure how useful as an abstraction to be included in core. If not we could just add them as Map and Set syntax extension.
not sure if, as abstractions, they are mature enough to be locked down in RC1.

Sorry, I am biased towards limiting scope in 1.0.

kailuowang · 2017-10-30T15:11:17Z

Can we make these two separate from the Traverse and Foldable. The way I see it they are parallel rather than hierarchical. Traverse and Foldable are for ordered data types, while these two are for unordered ones.

kailuowang · 2017-11-01T19:27:43Z

Can we rebase master? I am curious if this one is bin compat with 1.0.0-RC1, ~~Also I still feel pretty strongly that we should not have Traverse and Foldable inherit from them.~~

Update: I can see benefits of having UnorderedTraverse inheriting Traverse now.

hrhino · 2017-11-09T15:11:18Z

core/src/main/scala/cats/UnorderedTraverse.scala

+
+/**
+  * `UnorderedTraverse` is like a `Traverse` for unordered containers. In addition to the traverse and sequence
+  * methods it provides nonEmptyTraverse and nonEmptySequence methods which require an `Apply` instance instead of `Applicative`.


This line of docs seems inaccurate?

hrhino · 2017-11-09T15:14:45Z

core/src/main/scala/cats/UnorderedFoldable.scala

+    */
+  def forall[A](fa: F[A])(p: A => Boolean): Boolean =
+    unorderedFoldMap(fa)(p)(UnorderedFoldable.andMonoid)
+}


This could implement the size method from Foldable as well, I think.

hrhino · 2017-11-09T15:15:33Z

laws/src/main/scala/cats/laws/UnorderedFoldableLaws.scala

+    */
+  def forallEmpty[A](fa: F[A], p: A => Boolean): Boolean = {
+    !F.isEmpty(fa) || F.forall(fa)(p)
+  }


Could it be made to be such that existsLazy and forallLazy are laws for UnorderedFoldable and not just Foldable?

I think lazyness would require foldRight, no?

I believe you can do it with Eval: see my duplicative attempt at this type class.

def exists[A](fa: F[A])(p: A => Boolean): Boolean = foldMapUnordered(fa)(a => Eval.later(p(a)))(new CommutativeMonoid[Eval[Boolean]] { def empty = Eval.False def combine(x: Eval[Boolean], y: Eval[Boolean]): Eval[Boolean] = x.flatMap(xv => if (xv) Eval.True else y) }).value

and dually for forall.

You're absolutely right, this is great! Thank you so much!

hrhino · 2017-11-09T15:18:28Z

laws/src/main/scala/cats/laws/UnorderedTraverseLaws.scala

+
+import cats.data.Nested
+
+trait UnorderedTraverseLaws[F[_]] extends UnorderedFoldableLaws[F] {


This could/should extend FunctorLaws[F] and assert some things about the compatibility of UnorderedTraverse and Functor; for instance, I would expect unorderedSequence to be unorderedTraverse on identity (which assumes that Eq for these instances respects unorderedness if it's not also a Traverse, I think)

Hmmm, I'm not sure. That would imply that Set would be a valid Functor which I believe it is not. We could add a function like unorderedMap that's just unorderedTraverse with Id.

Thanks for your review by the way, it's been very helpful! :)

No, you're right; I got a little bit overenthusiastic about drawing these parallels.

I think unorderedMap is kinda dubious, because the difference isn't even lack of order (which is unobservable in Functor#map already!), but that the operation can "drop" values, losing fusion, and therefore isn't a (categorical) Functor. I don't know what laws for unorderedMap would be, other than "it's the same as traversing with Id".

… into add-unordered-classes

johnynek · 2017-11-22T02:17:43Z

laws/src/main/scala/cats/laws/UnorderedFoldableLaws.scala

+  }
+
+  def toSetRef[A](fa: F[A]): IsEq[Set[A]] =
+    F.unorderedFoldMap(fa)(a => Set(a)) <-> F.toSet(fa)


this could be

val lst1 = F.unorderedFoldMap(fa)(a => List(a)) val lst2 = F.toList(fa) def contains[A: Eq](a: A): Boolean = lst1.exists(Eq[A].eqv(_, a)) val allCont = list2.map(contains) val allTrue = lst.map(_ => true) allCont <-> allTrue

In this way, we have a law that only needs Eq, not unusual, and the typeclass does not require any Order or Hash.
if we have an Order[A]. So we use an Order to check the law more quickly, or we could

… into add-unordered-classes

Luka Jacobowitz and others added 21 commits September 24, 2017 18:22

Add CommutativeApply and Applicative and an instance for Validated[Co…

0ea4ac0

…mmutativeSemigroup, ?]

Make Future a CommutativeApplicative

0cd4eb9

Revert "Make Future a CommutativeApplicative"

f9a239a

This reverts commit 0cd4eb9.

Add unordered traversal for Sets

965f9c2

Test commutativeApply instances + Nested + Tuple2K instances

d3a803a

Add unordered traversal for Map

1ab4f47

Fix priority for Nested instances

2164b51

Fix priority for Tuple2K instances

7f1f6c7

Merge branch 'master' into add-commutative-apply

75a656a

Merge branch 'master' into add-commutative-apply

69c6374

Move methods to typeclass

fa8a731

Deduplicate Applicative instance

2aeddd7

Try out new typeclasses

aa49de9

Add UnorderedFoldable and UnorderedTraverse and move traversal functi…

d308cf7

…ons there

Merge branch 'master' into add-commutative-apply

148c113

Remove Set from Foldable docs

0ffbc55

Add extra ref law

23cb589

Merge branch 'master' into add-commutative-apply

e0069a8

Revert "Add UnorderedFoldable and UnorderedTraverse and move traversa…

376a5c6

…l functions there" This reverts commit d308cf7.

Revert "Remove Set from Foldable docs"

d673655

This reverts commit 0ffbc55.

Add Unordered type classes

8666c64

LukaJCB added the Source Breaking label Oct 19, 2017

stew added the in progress label Oct 19, 2017

LukaJCB mentioned this pull request Oct 19, 2017

Add CommutativeApply and CommutativeApplicative #1927

Merged

Make UnorderedFoldable and Traverse super classes

74b0228

LukaJCB force-pushed the add-unordered-classes branch from 81feb52 to 74b0228 Compare October 20, 2017 00:53

Luka Jacobowitz added 2 commits October 31, 2017 12:29

Merge branch 'master' into add-unordered-classes

3e705d4

Add mention to unorderedFoldable in foldable scaladoc

5f4d41f

johnynek previously approved these changes Oct 31, 2017

View reviewed changes

LukaJCB added this to the 1.0.0 milestone Nov 1, 2017

kailuowang mentioned this pull request Nov 9, 2017

[WIP] Add an UnorderedFoldable that we can give instances for Set/Map/&c #2021

Closed

Merge branch 'master' into add-unordered-classes

cd2e1f5

kailuowang dismissed johnynek’s stale review via cd2e1f5 November 9, 2017 14:59

hrhino reviewed Nov 9, 2017

View reviewed changes

Luka Jacobowitz added 2 commits November 10, 2017 12:03

Add size to UnorderedFoldable

5d9f0f7

Merge branch 'add-unordered-classes' of https://github.com/LukaJCB/cats…

f70fe6f

… into add-unordered-classes

LukaJCB force-pushed the add-unordered-classes branch from 5523596 to f70fe6f Compare November 10, 2017 16:14

kailuowang added the enhancement label Nov 10, 2017

Luka Jacobowitz and others added 6 commits November 14, 2017 12:50

Made exists and forall sufficiently lazy

04722f0

Add Mima exceptions

9568933

Merge branch 'master' into add-unordered-classes

9666a0c

Merge branch 'master' into add-unordered-classes

23a6ca1

Merge branch 'master' into add-unordered-classes

d2037e6

Fix merging mistake

11b397c

johnynek reviewed Nov 22, 2017

View reviewed changes

Remove toSet

89cab23

kailuowang previously approved these changes Nov 24, 2017

View reviewed changes

Merge branch 'add-unordered-classes' of https://github.com/LukaJCB/cats…

e2984b4

… into add-unordered-classes

LukaJCB dismissed kailuowang’s stale review via e2984b4 November 24, 2017 15:09

LukaJCB force-pushed the add-unordered-classes branch from d0ac64e to e2984b4 Compare November 24, 2017 15:09

Remove unused imports

ea38cd6

johnynek approved these changes Nov 24, 2017

View reviewed changes

kailuowang approved these changes Nov 25, 2017

View reviewed changes

kailuowang merged commit d4a3cf5 into typelevel:master Nov 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UnorderedFoldable and UnorderedTraverse #1981

Add UnorderedFoldable and UnorderedTraverse #1981

LukaJCB commented Oct 19, 2017 •

edited

Loading

codecov-io commented Oct 19, 2017 •

edited

Loading

kailuowang commented Oct 20, 2017

LukaJCB commented Oct 20, 2017

kailuowang commented Oct 20, 2017

kailuowang commented Oct 30, 2017

kailuowang commented Nov 1, 2017 •

edited

Loading

hrhino Nov 9, 2017

hrhino Nov 9, 2017

hrhino Nov 9, 2017

LukaJCB Nov 10, 2017

hrhino Nov 10, 2017

LukaJCB Nov 14, 2017

hrhino Nov 9, 2017

LukaJCB Nov 10, 2017

hrhino Nov 10, 2017 •

edited

Loading

johnynek Nov 22, 2017


		import cats.data.Nested

		trait UnorderedTraverseLaws[F[_]] extends UnorderedFoldableLaws[F] {

Add UnorderedFoldable and UnorderedTraverse #1981

Add UnorderedFoldable and UnorderedTraverse #1981

Conversation

LukaJCB commented Oct 19, 2017 • edited Loading

codecov-io commented Oct 19, 2017 • edited Loading

Codecov Report

kailuowang commented Oct 20, 2017

LukaJCB commented Oct 20, 2017

kailuowang commented Oct 20, 2017

kailuowang commented Oct 30, 2017

kailuowang commented Nov 1, 2017 • 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

hrhino Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaJCB commented Oct 19, 2017 •

edited

Loading

codecov-io commented Oct 19, 2017 •

edited

Loading

kailuowang commented Nov 1, 2017 •

edited

Loading

hrhino Nov 10, 2017 •

edited

Loading