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

foldLeftM without Free. #1117

Merged
merged 1 commit into from
Jun 21, 2017
Merged

foldLeftM without Free. #1117

merged 1 commit into from
Jun 21, 2017

Conversation

TomasMikula
Copy link
Contributor

@TomasMikula TomasMikula commented Jun 11, 2016

This is an implementation of foldLeftM without using Free. Instead, it introduces an auxiliary structure

case class Source[A](uncons: () => Option[(A, Source[A])])

and a conversion from any foldable F[A] to Source[A], which provides lazy access to elements of F[A].

It still uses foldRight, but I think there is a better separation of concerns here, because G is not involved in the foldRight at all.

I didn't bother to move this out of Free.scala, but Free is not used here at all. I moved the implementation to Foldable.

Funny enough, if I make Source extend AnyVal, it crashes the compiler with a StackOverflowError (SI-9600).

@codecov-io
Copy link

codecov-io commented Jun 11, 2016

Codecov Report

Merging #1117 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1117      +/-   ##
==========================================
- Coverage   93.99%   93.98%   -0.02%     
==========================================
  Files         249      249              
  Lines        4131     4140       +9     
  Branches      158      160       +2     
==========================================
+ Hits         3883     3891       +8     
- Misses        248      249       +1
Impacted Files Coverage Δ
free/src/main/scala/cats/free/Free.scala 84% <ø> (-2.54%) ⬇️
core/src/main/scala/cats/Foldable.scala 98.75% <100%> (+0.19%) ⬆️

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 3ba8993...eab2053. Read the comment docs.

@ceedubs
Copy link
Contributor

ceedubs commented Jun 13, 2016

@TomasMikula thanks, this does seem quite a bit simpler than the Free approach. This week I'll try to take some time to figure out which other methods we want better stack-safe/lazy support for and whether we can do something similar there as opposed to needing to bring Free into core.

@TomasMikula
Copy link
Contributor Author

TomasMikula commented Jun 13, 2016

This approach basically gives an Iterator-like/Stream-like interface to (any) Foldable, which could potentially be useful elsewhere. Also, I think pretty much any concrete foldable data type will have a much more efficient implementation of foldLeftM (or "toStream") than this default one using foldRight.

@ceedubs
Copy link
Contributor

ceedubs commented Jun 16, 2016

@TomasMikula I like this. I'd be interested in moving it into core somewhere where we could use it in the default implementation of Foldable.foldLeftM I think (though we could maybe leave it private for now so it doesn't become a part of the official cats API for now). I'd also be interested in some benchmarks to see whether there's any performance difference if you were to use an abstract class with an abstract def as opposed to a case class with a Function0 val. I'm happy to put these together, but I also don't want to steal your thunder if this is something that you want to see through. WDYT?

@ceedubs
Copy link
Contributor

ceedubs commented Jun 16, 2016

cc @non since he and I have talked about foldLeftM and the possibility of moving Free to core to get the functionality that this achieves.

@TomasMikula
Copy link
Contributor Author

@ceedubs I made the changes. I made Source to be an abstract class. I was originally going for a value class, but hit SI-9600.

@TomasMikula
Copy link
Contributor Author

TomasMikula commented Jun 16, 2016

Perhaps we need better naming for foldM vs. foldLeftM? They are both left folds. They differ in the ability to short-circuit.

TomasMikula referenced this pull request in TomasMikula/cats Jun 16, 2016
I don't like the Eval.value calls, but I don't know of a way around
them. The unit tests suggest that they aren't a problem due to the
nature of `Free.runTailRec` and `FlatMapRec.tailRecM`.
test(".foldLeftM") {
// you can see .foldLeftM traversing the entire structure by
// changing the constant argument to .take and observing the time
// this test takes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment should no longer be true, should it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it became obsolete earlier, though. See my comment. The whole test case is redundant in favor of the one below.

@ceedubs
Copy link
Contributor

ceedubs commented Jun 18, 2016

Perhaps we need better naming for foldM vs. foldLeftM? They are both left folds. They differ in the ability to short-circuit.

@TomasMikula I agree. I think we are going to likely end up having several methods that have a Monad form and a separate MonadRec form. Maybe to fit the names of those two type classes we could use the convention of foldLeftM and foldLeftMRec?

F.foldRight[A, Source[A]](fa, Now(Source.empty[A]))((a, evalSrc) =>
Now(cons(a, evalSrc))
).value
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably me prematurely optimizing, but I'd be tempted to do a couple things:

  • Make empty reuse a singleton either by making Source covariant or by giving it a def widen[AA >: A]: Source[AA] = this.asInstanceOf[Source[AA]] method.
  • Possibly store a value of Now(Source.empty) around for reuse by fromFoldable.

@TomasMikula
Copy link
Contributor Author

TomasMikula commented Jun 18, 2016

@ceedubs I made the changes, except renaming. I like the -Rec suffix convention. Do you mean we should rename foldM to foldLeftM? Or the current foldLeftM to foldMRec?

@ceedubs
Copy link
Contributor

ceedubs commented Jun 19, 2016

@TomasMikula I was thinking about renaming the current foldM to foldLeftM and having this new one be foldLeftMRec. I guess if we don't have a right version then there is no harm in foldM and foldMRec, but I think it makes it a little more obvious that it will be left-associated and parallels the foldLeft name better. WDYT?

@TomasMikula
Copy link
Contributor Author

@ceedubs Makes sense. My concern was just whether foldM was a standard name for this operation. Probably not. scalaz has foldLeftM, Haskell has foldlM.

@TomasMikula
Copy link
Contributor Author

How about now?

@ceedubs
Copy link
Contributor

ceedubs commented Jun 19, 2016

👍 thanks @TomasMikula!

It might be nice to have the foldLeftM and foldLeftMRec scaladocs reference each other to point out why people might want one or the other. Also we'll probably want to optimize the implementations in some of our instances. But I think all of that can wait for subsequent PRs. I'd rather get this merged and then iterate.

@kailuowang
Copy link
Contributor

kailuowang commented Jun 6, 2017

@TomasMikula I am sorry that this fell through the cracks, any interest in resolving the conflicts? It looks like that we are close to merging. If you don't have time I'd be happy to continue it through.

@kailuowang kailuowang mentioned this pull request Jun 6, 2017
26 tasks
*/
def foldM[G[_], A, B](fa: F[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] =
def foldLeftM[G[_], A, B](fa: F[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need this now, right?

* have a more efficient implementation than the default one
* in terms of `foldRight`.
*/
def foldLeftMRec[G[_], A, B](fa: F[A], z: B)(f: (B, A) => G[B])(implicit G: MonadRec[G]): G[B] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we rename this foldLeftM or just stick with foldM. I'm not sure we should be changing this name this late in the game, although I agree foldLeftM is a better name.

Maybe add a deprecated final foldM that redirects to foldLeftM?

@TomasMikula TomasMikula force-pushed the foldLeftM branch 2 times, most recently from ab7d173 to e6b28b4 Compare June 17, 2017 22:55
@TomasMikula
Copy link
Contributor Author

How about now?

val NowEmpty: Eval[Source[Nothing]] = Now(Empty)

def cons[A](a: A, src: Eval[Source[A]]): Source[A] = new Source[A] {
def uncons = Some((a, src.value))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little nervous about these calls to value. Can we add a comment about the stack safety here? I think the answer is that we can't recursively call .value in an Eval and we should never be in an Eval when we call uncons, is that right?

Copy link
Contributor Author

@TomasMikula TomasMikula Jun 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The presence of Eval in this case actually just obstructs the understanding, IMO. The src parameter of cons could just be by-name (the evaluation of .value doesn't need any trampolining built into Eval). However, since foldRight already uses Eval, I didn't want to mix both by-name and Eval.

A clearer definition of Source would be

type Source[A] = () => Option[(A, Source[A])]

except that recursive type aliases are not allowed. I guess I could make it an AnyVal, though. EDIT: Nope, because of scala/bug#9600.

* in terms of `foldRight`.
*/
def foldLeftM[G[_], A, B](fa: F[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] = {
val src = Foldable.Source.fromFoldable(fa)(self)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of a bummer that is someone has override on foldM they will have two different folds. Maybe one should be final and a pointer to the other?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True.

@TomasMikula
Copy link
Contributor Author

TomasMikula commented Jun 18, 2017

I realized that in case of short-circuited fold, it evaluates 1 more element than necessary. Maybe I should fix that...

EDIT: Note that because of the signature of foldM

def foldM[G[_], A, B](fa: F[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B]

specifically the part z: B, we can never short-circuit before evaluating at least 1 element. If instead it was z: G[B], there would be a possibility to short-circuit without evaluating any elements at all.

@johnynek
Copy link
Contributor

👍

@kailuowang kailuowang merged commit e3a969e into typelevel:master Jun 21, 2017
@kailuowang
Copy link
Contributor

merged with 3 sign-offs

@TomasMikula TomasMikula deleted the foldLeftM branch June 21, 2017 18:29
assert(res == Left(100000))
}

test(".foldLeftM short-circuiting optimality") {
Copy link
Contributor

@kailuowang kailuowang Jun 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails if we remove Foldable.iteratorFoldM, because it actually no longer tests the default foldM implementation from Foldable after #1414 which overrides foldLeftM in the instance of Stream using iteratorFoldM. This test fails when using the default foldM because, if I am not mistaken, the new foldM still have to evaluate one element after the stop. Also these two tests are now redundant with the test("Foldable[Stream]") above. I will fix this in a PR removing iteratorFoldM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, I didn't notice that I was testing some other code 😃. I suppose changing the signature of

def uncons: Option[(A, Source[A])]

to

def uncons: Option[(A, Eval[Source[A]])]

and doing the necessary changes would make it pass.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, I updated #1740

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants