Lazy foldM for "Iterables" #1414

peterneyens · 2016-10-19T09:19:15Z

If I am understanding correctly, now that FlatMap has tailRecM we can revisit #1030.

I adapted an old commit by @ceedubs for a PR to add MonadRec by @TomasMikula.

Resolves typelevel#1030, though it may be desirable change the default implementation to be lazy in the future. This can be done with `FreeT` but as of now I'm not sure how to do it without.

codecov-io · 2016-10-19T10:26:03Z

Current coverage is 91.67% (diff: 100%)

Merging #1414 into master will decrease coverage by <.01%

@@             master      #1414   diff @@
==========================================
  Files           240        240          
  Lines          3608       3617     +9   
  Methods        3540       3555    +15   
  Messages          0          0          
  Branches         67         61     -6   
==========================================
+ Hits           3308       3316     +8   
- Misses          300        301     +1   
  Partials          0          0

Powered by Codecov. Last update 7ea2024...6f3da09

TomasMikula · 2016-10-19T15:20:40Z

Some structures, such as Vector or a balanced tree, allocate new memory in tail, but usually not in Iterator.next. Thus I believe that an implementation that uses an iterator instead of repeatedly calling tail can be significantly more efficient in some cases.

kailuowang · 2016-10-21T15:05:21Z

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

@@ -372,4 +378,12 @@ object Foldable {
      Eval.defer(if (it.hasNext) f(it.next, loop()) else lb)
    loop()
  }
+
+  def iteratorFoldM[M[_], A, B](it: Iterator[A], z: B)(f: (B, A) => M[B])(implicit M: Monad[M]): M[B] = {


How about we also mention here in the doc the incentive of this special implementation for Iteratables?

non · 2016-10-21T19:34:56Z

The code seems good to me. 👍 Thanks!

TomasMikula · 2016-10-21T19:39:28Z

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

+  def iteratorFoldM[M[_], A, B](it: Iterator[A], z: B)(f: (B, A) => M[B])(implicit M: Monad[M]): M[B] = {
+    val go: ((B, Iterator[A])) => M[Either[(B, Iterator[A]), B]] = { case (b, it) =>
+      if (it.hasNext) M.map(f(b, it.next))(b1 => Left((b1, it)))
+      else M.pure(Right(b))


Since Iterator is mutable, I would keep it private to the method (otherwise we lose referential transparency). I would keep the signature as you had before, and only use Iterator inside.

Sorry if this was not clear from my previous comment.

TomasMikula · 2016-10-21T19:44:24Z

Not sure what I did wrong that in this conversation it shows my newest comment as outdated. I will repeat here:

Since Iterator is mutable, I would keep it private to the method (otherwise we lose referential transparency). I would keep the signature as you had before, and only use Iterator inside.

Sorry if this was not clear from my previous comment.

non · 2016-10-21T19:45:16Z

@TomasMikula in the past we've exposed other methods using Iterator to make it easier for third-parties to build their instances (e.g. iterateRight). We could have the method take something like () => Iterator[A] if that makes it seem better.

TomasMikula · 2016-10-21T19:50:32Z

Well, in that case, ship it!

TomasMikula · 2016-10-21T19:51:37Z

Another, perhaps premature, optimization would be to avoid allocating the tuple in each step by accessing the iterator from the outer scope instead of passing it along.

TomasMikula · 2016-10-21T22:24:20Z

LGTM 👍

kailuowang · 2016-10-22T00:34:11Z

👍 thanks very much!

johnynek · 2016-10-25T18:12:05Z

Yeah, I'm concerned everything except List will be slower here.
On Wed, Oct 19, 2016 at 05:20 Tomas Mikula notifications@github.com wrote:

Some structures, such as Vector or a balanced tree, allocate new memory
in tail, but usually not in Iterator.next. Thus I believe that an
implementation that uses an iterator instead of repeatedly calling tail
can be significantly more efficient in some cases.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#1414 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEJdqvkN0GhLFT8WkSM-O-bt1_AyOh3ks5q1jVIgaJpZM4KavX1
.

ceedubs and others added 2 commits October 19, 2016 11:15

Add foldMRec

2cc1122

Resolves typelevel#1030, though it may be desirable change the default implementation to be lazy in the future. This can be done with `FreeT` but as of now I'm not sure how to do it without.

Lazy foldM for "Iterables" after FlatMap.tailRec.

6bc8f58

stew added the in progress label Oct 19, 2016

Use Iterator instead of Iterable for laxy foldM

52c3b7f

kailuowang reviewed Oct 21, 2016

View reviewed changes

TomasMikula reviewed Oct 21, 2016

View reviewed changes

Document and optimize Foldable.iteratorFoldM

6f3da09

peterneyens force-pushed the lazy-foldM branch from 381aa3d to 6f3da09 Compare October 21, 2016 22:24

kailuowang merged commit f90245d into typelevel:master Oct 22, 2016

peterneyens removed the in progress label Oct 22, 2016

peterneyens deleted the lazy-foldM branch October 22, 2016 00:45

peterneyens mentioned this pull request Jun 3, 2017

Foldable instance for List breaks substitution #1716

Closed

kailuowang mentioned this pull request Jun 23, 2017

foldLeftM without Free. #1117

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy foldM for "Iterables" #1414

Lazy foldM for "Iterables" #1414

peterneyens commented Oct 19, 2016

codecov-io commented Oct 19, 2016 •

edited

Loading

TomasMikula commented Oct 19, 2016

kailuowang Oct 21, 2016

non commented Oct 21, 2016

TomasMikula Oct 21, 2016

TomasMikula Oct 21, 2016

TomasMikula commented Oct 21, 2016

non commented Oct 21, 2016 •

edited

Loading

TomasMikula commented Oct 21, 2016

TomasMikula commented Oct 21, 2016

TomasMikula commented Oct 21, 2016

kailuowang commented Oct 22, 2016

johnynek commented Oct 25, 2016

Lazy foldM for "Iterables" #1414

Lazy foldM for "Iterables" #1414

Conversation

peterneyens commented Oct 19, 2016

codecov-io commented Oct 19, 2016 • edited Loading

Current coverage is 91.67% (diff: 100%)

TomasMikula commented Oct 19, 2016

kailuowang Oct 21, 2016

Choose a reason for hiding this comment

non commented Oct 21, 2016

TomasMikula Oct 21, 2016

Choose a reason for hiding this comment

TomasMikula Oct 21, 2016

Choose a reason for hiding this comment

TomasMikula commented Oct 21, 2016

non commented Oct 21, 2016 • edited Loading

TomasMikula commented Oct 21, 2016

TomasMikula commented Oct 21, 2016

TomasMikula commented Oct 21, 2016

kailuowang commented Oct 22, 2016

johnynek commented Oct 25, 2016

codecov-io commented Oct 19, 2016 •

edited

Loading

non commented Oct 21, 2016 •

edited

Loading