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

LazyFoldRight #225

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@EPronovost
Copy link
Contributor

EPronovost commented Sep 4, 2017

Per #186

@julienrf

This comment has been minimized.

Copy link
Contributor

julienrf commented Sep 8, 2017

This looks correct but you should add tests. Also, I think it would be interesting to have this method on IterableOps instead of only LazyList.

Move lazyFoldRight to IterableOps
Also add junit tests
@EPronovost

This comment has been minimized.

Copy link
Contributor

EPronovost commented Sep 8, 2017

How would I ensure it's stack safe? Both foldRight and lazyFoldRight should be.

@Ichoran

This comment has been minimized.

Copy link
Contributor

Ichoran commented Sep 9, 2017

Just include a test that is 100,000 elements long. If it's not stack-safe, it'll blow the stack and the test will fail.

@EPronovost

This comment has been minimized.

Copy link
Contributor

EPronovost commented Sep 10, 2017

The current implementations are not stack safe. Looking into similar implementations in cats, it looks like a different signature is required for such an implementation. Is that the route we want to go?

@EPronovost EPronovost force-pushed the EPronovost:lazyFoldRight branch from 38cffb9 to 34fdbed Sep 10, 2017

@julienrf

This comment has been minimized.

Copy link
Contributor

julienrf commented Sep 12, 2017

Hey @EPronovost,

I think we should implement foldRight in Iterator as follows:

def foldRight[B](z: B)(op: (A, B) => B): B =
  reversed.foldLeft(z)((b, a) => op(a, b))

Where reversed is copy-pasted from IterableOps. At some point we might want to reduce the duplication between Iterator and Iterable, but that’s a different work :)

We should also re-implement foldRight in IterableOps like that (to benefit from the fact that reversed is overriden in IndexedSeq and is O(1)) instead of delegating to iterator().foldRight….

For lazyFoldRight, it seems that we indeed have to change its signature in order to implement it in stack-safe way :(

Enforce stack safety
Restructure `foldRight` to be stack safe with `reverse` (to take advantage of collections with O(1) reversal)
Switch `lazyFoldRight` to the different signature.
@julienrf

This comment has been minimized.

Copy link
Contributor

julienrf commented Sep 13, 2017

Hi @EPronovost! First, thanks for your patience because we are groping together :) What do you think of having a more meaningful type rather than Either[B, B => B]?

What do you think of the following?

/**
  * A folding step consists either in stopping the traversal (`Break`) or
  * getting the result of folding the tail of the collection (`Pull`).
  */
sealed trait FoldStep[A]

object FoldStep {
  case class Break[A](value: A) extends FoldStep[A]
  case class Pull[A](f: A => A) extends FoldStep[A]
}

And then:

def lazyFoldRight[B](z: B)(f: A => FoldStep[B]): B = {
  def foldSteps(x: B, fs: List[B => B]): B = fs.foldLeft(x)((b, f) => f(b))

  def loop(fs: List[B => B]): B =
    if (hasNext) {
      f(next()) match {
        FoldStep.Done(b) => foldSteps(b, fs)
        FoldStep.Pull(f) => loop(f :: fs)
      }
    } else foldSteps(z, fs)

  loop(Nil)
}

Example of use:

def forall[A](as: Iterable[A], p: A => Boolean): Boolean =
  as.lazyFoldRight(true)(a => if (!p(a)) FoldStep.Break(false) else FoldStep.Pull(b => b))
@EPronovost

This comment has been minimized.

Copy link
Contributor

EPronovost commented Sep 13, 2017

@julienrf That's definitely a more meaningful type usage. My potential concern is in introducing a whole new trait for this one specific use case. Is it worth it?

@Ichoran

This comment has been minimized.

Copy link
Contributor

Ichoran commented Sep 13, 2017

What about lazyFoldRight[B](zero: B)(indep: A => Option[B])(accum: (A, B) => B)? You then run indep until either you get a Some(b) or you start with zero; and then you do a normal right fold with accum on that fragment? The algorithm is then easy:

var xs: List[A] = Nil
var z: Option[B] = None
while (hasNext && (z eq None)) {
  val a = next
  z = indep(a)
  if (z.isEmpty) xs = a :: xs
}
xs.foldLeft(z getOrElse zero)((b,a) => accum(a,b))

(Even easier if accum has the opposite order of parameters from normal.)

@Ichoran

This comment has been minimized.

Copy link
Contributor

Ichoran commented Sep 13, 2017

As an aside, I'm not really convinced that this method is general-purpose enough to include.

You can xs.takeWhile(dependent).foldRight(zero)(op) as long as you're dealing with a monoid. Unless it really is the ability to synthesize of chain of different B => Bs that is critical, and in that case the usage is even more specialized.

@julienrf

This comment has been minimized.

Copy link
Contributor

julienrf commented Sep 14, 2017

Yeah, it’s right that we can always use takeWhile to not consume the whole collection. Do you have examples of things that you can not express with takeWhile + foldRight but can with lazyFoldRight?

@EPronovost

This comment has been minimized.

Copy link
Contributor

EPronovost commented Sep 14, 2017

I don't think there's anything you can do with one and not the other, but takeWhile + foldRight would require two iterations, and thus be less performant. I can't think of a way to do "foldRight on an unknown-length prefix of the collection" in a single iteration with the existing methods

@Ichoran

This comment has been minimized.

Copy link
Contributor

Ichoran commented Sep 14, 2017

Traversing twice is almost always faster than building a list of closures (two mandatory object creations per element).

@EPronovost

This comment has been minimized.

Copy link
Contributor

EPronovost commented Sep 15, 2017

@Ichoran @julienrf so then what do we want to do for lazyFoldRight()?

@EPronovost EPronovost referenced this pull request Sep 15, 2017

Closed

Fold left while #228

@julienrf

This comment has been minimized.

Copy link
Contributor

julienrf commented Sep 20, 2017

On View and LazyList, takeWhile doesn’t build a new collection. So :

(1 :: 2 :: 3 :: Nil).view.takeWhile(p).foldRight(z)(op)

and

(1 :: 2 :: 3 :: Nil).lazyFoldRight(z)(x => if (!p(x)) Break(z2) else Pull(y => op(x, y)))

Would be equivalent. A key difference between both is that with lazyFoldRight you can provide a value for the element that doesn’t verify the predicate p. We could probably achieve the same with a takeUntil method, though.

Let’s compare the implementation of forall with takeUntil + foldRight, the lazyFoldRight signature I proposed and the one proposed by @Ichoran:

// With takeWhile + foldRight
def forall[A](xs: Iterable[A])(p: A => Boolean): Boolean =
  xs.view.takeUntil(a => !p(a)).foldRight(true)((a, b) => b && p(a))

// lazyFoldRight
def forall[A](xs: Iterable[A])(p: A => Boolean): Boolean =
  xs.lazyFoldRight(true)(a => if (p(a)) Pull(b => b) else Break(false))

// Ichoran’s lazyFoldRight
def forall[A](xs: Iterable[A])(p: A => Boolean): Boolean =
  xs.lazyFoldRight(true)(a => if (p(a) None else Some(false))((a, b) => b && p(a))

I would argue that the lazyFoldRight form that I proposed might be preferable because we both compute the B and control the traversal at the same time (p(a) is evaluated only once). In the alternatives we might have to perform redundant computations.

@julienrf

This comment has been minimized.

Copy link
Contributor

julienrf commented Sep 27, 2017

Hey @EPronovost, could you move lazyFoldRight to a decorator, in collections-contrib?

@julienrf

This comment has been minimized.

Copy link
Contributor

julienrf commented Nov 15, 2017

Superseded by #295

@julienrf julienrf closed this Nov 15, 2017

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