Add Iterator.unfold and IterableFactory.unfold #6851

NthPortal · 2018-06-24T04:33:10Z

Ichoran

Algorithm assumes immutability, but it shouldn't.

Ichoran · 2018-06-24T21:05:02Z

src/library/scala/collection/Iterator.scala

@@ -1,6 +1,7 @@
 package scala.collection

 import java.io.{ObjectInputStream, ObjectOutputStream}
+import java.util.Objects


The method you're using is so trivial that I don't think it's worth adding this dependency.

Ichoran · 2018-06-24T21:13:25Z

src/library/scala/collection/Iterator.scala

+
+    override def hasNext: Boolean = {
+      if (nextResult eq null) {
+        nextResult = Objects.requireNonNull(f(state), "null during unfold")


This logic isn't safe if f is side-effecting--for instance, if it generates a random sequence that terminates. You need to have a separate var that remembers when you've hit the last item or somesuch. Personally I'd encode it as a uninitialized bool, and just put (A, S) in its own var with it being null as the indication that the stream has stopped (and then pack init into (null.asInstanceOf[A], init) to get going).

Or better yet use two fields, one for S and one for A, and either bools or integer state to keep track of what's going on.

I'm not sure I understand where it's unsafe. None indicates that the stream has stopped

Huh. I saw that. And I thought I saw a scenario where it wouldn't work, but I'm unable to recreate it. I suspect I got confused somehow.

Ichoran · 2018-06-24T21:25:02Z

src/library/scala/collection/Iterator.scala

+
+    override def hasNext: Boolean = {
+      if (nextResult eq null) {
+        nextResult = Objects.requireNonNull(f(state), "null during unfold")


Huh. I saw that. And I thought I saw a scenario where it wouldn't work, but I'm unable to recreate it. I suspect I got confused somehow.

NthPortal · 2018-06-24T21:29:40Z

src/library/scala/collection/Iterable.scala

+    * @tparam S   Type of the internal state
+    */
+  def unfold[A, S](init: S)(f: S => Option[(A, S)]): Iterable[A] = {
+    val initialState = init // unfortunately, `Iterable` has an `init` method


This aliasing bugs me, but I'm honestly not sure what the best course of action is.

init is a good, short parameter name, and I'm not a huge fan of a longer name like initialState. Ideally, all of the unfold methods ought to have the same parameter names, so changing this one means changing LazyList.unfold and Iterator.unfold as well.

Perhaps lengthen it slightly to initial?

Let the bikeshedding commence!

What if you @inline def initialState = init?

Ichoran · 2018-06-24T21:48:35Z

I tested it, and the @inline def doesn't actually work--the call is inlined, but the anonymous inner class still picks up a reference to the enclosing class as if it needed to call the method.

The local val is fine as a rename, but having a non-anonymous class generates slightly cleaner bytecode to begin with. (In particular, it avoids an extra

      3: dup
      4: aload_0

). This is probably JITted out immediately, though. (I didn't check the assembly.)

julienrf · 2018-06-25T08:19:16Z

src/library/scala/collection/Iterable.scala

+    */
+  private final class UnfoldIterable[A, S](initial: S)(f: S => Option[(A, S)]) extends AbstractIterable[A] {
+    override def iterator: Iterator[A] = Iterator.unfold(initial)(f)
+  }


What do you think of adding it to IterableFactory (and SortedIterableFactory, MapFactory, SortedMapFactory) instead, so that this would be available on all collections types?

Also, instead of using an UnfoldIterable class, you can just call from(Iterator.unfold(initial)(f)) (though this would be less efficient than your version).

UnfoldIterable doesn't memoize items since it creates new UnfoldIterator every time iteration is done. Iterable.from just creates an immutable List IIRC. It depends what we want.

I should probably document Iterable.unfold better to indicate that it doesn't memoize elements

I've checked scala.collection.View and it seems a better fit than scala.collection.Iterable to put collections without memoization. Views are not memoized by default, while Iterables are backed by immutable.List by default. Instead of changing the semantics for one method (unfold) in Iterable companion object, we can move that method (unfold) to View companion object. WDYT? @LPTK

If we add a View.Unfold class, then we can have Factory.from(new View.Unfold(initial)(f)) for all types basically for free - similarly to what's done with View.Iterate, View.Tabulate, View.Fill, etc.

julienrf · 2018-06-25T08:39:33Z

Also, maybe we should replace LazyList.unfold’s implementation with LazyList.fromIterator(Iterator.unfold(initial)(f)). That would address the fact that the current implementation of LazyList.unfold is not tail recursive.

julienrf · 2018-06-28T05:34:05Z

Yes, I think having View.unfold is the way to go.

…

On June 28, 2018 5:42:36 AM GMT+02:00, Nth ***@***.***> wrote: NthPortal commented on this pull request. > + * @return an Iterable by using a function `f` producing elements of + * type `A` and updating an internal state `S`. + * @param init State initial value + * @param f Computes the next element (or returns `None` to signal + * the end of the collection) + * @tparam A Type of the elements + * @tparam S Type of the internal state + */ + def unfold[A, S](init: S)(f: S => Option[(A, S)]): Iterable[A] = new UnfoldIterable(init)(f) + + /** An `Iterable` that calls [[scala.collection.Iterator.unfold `Iterator.unfold`]] + * to create `Iterator`s. + */ + private final class UnfoldIterable[A, S](initial: S)(f: S => Option[(A, S)]) extends AbstractIterable[A] { + override def iterator: Iterator[A] = Iterator.unfold(initial)(f) + } If we add a `View.Unfold` class, then we can have `Factory.from(new View.Unfold(initial)(f))` for all types basically for free

NthPortal · 2018-07-01T21:08:17Z

I think this is ready for merge, if anyone wants to re-review

julienrf · 2018-07-02T08:21:38Z

test/junit/scala/SerializationStabilityTest.scala

@@ -81,7 +81,7 @@ object SerializationStability {
    }
  }

-  // Generated on 20180605-18:45:47 with Scala version 2.13.0-20180604-234247-8dd6ca5)
+  // Generated on 20180701-21:01:46 with Scala version 2.13.0-20180701-205044-6e3f96b)


Should we override this file?

I'm not sure I understand what you're asking

Why was this file overridden? That’s the first time I see it.

The changes broke the test, so I had to update the test. Doing so automatically also updates the timestamp and hash

I'm not actually sure why the changes broke the test, since theoretically they only affect factories and companion objects, which shouldn't store any serialization data

This is safe to regenerate at the moment. All collection classes have new SerialVersionUIDs and are serialization incompatible with M3 and earlier versions anyway.

julienrf · 2018-07-02T08:22:35Z

test/junit/scala/collection/IteratorTest.scala

+
+  @Test def unfold(): Unit = {
+    val it = Iterator.unfold(1)(i => if (i > 10) None else Some((i, i + 1)))
+    assertSameElements(1 to 10, it)


Do you mind adding a test that produces an empty Iterator?

What about a test checking that creation of Iterator.UnfoldIterator and invoking View.Unfold.iterator doesn't invoke generator function (i.e. one that takes state and produces optionally next element and state)? LazyList.unfold doesn't have this property - it always invokes generator function once, even if the LazyList is never used after LazyList.unfold.

@tarsa that's a good idea - can also do that :)

julienrf · 2018-07-02T08:28:14Z

src/library/scala/collection/Factory.scala

+    * @tparam S   Type of the internal state
+    * @return a $coll that produces elements using `f` until `f` returns `None`
+    */
+  def unfold[A : Ev, S](init: S)(f: S => Option[(A, S)]): CC[A] = from(new View.Unfold(init)(f))


If we add it to IterableFactory and EvidenceIterableFactory we should also add it to SortedIterableFactory, MapFactory and SortedMapFactory.

Or we can remove it from XxxFactory types and let users write Iterator.unfold(…)(…).to(Xxx) instead.

I attempted to be consistent with how iterate, tabulate and fill were defined.

Also, if we remove it from IterableFactory, then it's not defined for any factories at all, which seems undesirable.

OK, I’m fine with this argument.

@julienrf IMHO it would be best not to expect users to rely on iterators for common useful operations, as iterators should be regarded as an error-prone (stateful) lowish-level abstraction, mainly used as implementation details.

@LPTK Then you can just write: View.unfold(…)(…).to(Xxx) :)

@julienrf you can't, because object View inherits unfold from IterableFactory. You could do new View.Unfold(...)(...).to(Xxx), but that seems not great.

szeiger · 2018-07-06T16:50:12Z

src/library/scala/collection/immutable/LazyList.scala

-    * @tparam A   Type of the elements
-    * @tparam S   Type of the internal state
-    */
-  def unfold[A, S](init: S)(f: S => Option[(A, S)]): CC[A] = {


Is the new generic implementation as efficient as this one and sufficiently lazy?

it certainly should be sufficiently lazy, since it just creates a LazyList from a View. I can't speak to its performance (I haven't done any benchmarks), but I wouldn't imagine it to be substantially different.

szeiger · 2018-07-06T16:51:31Z

test/junit/scala/SerializationStabilityTest.scala

@@ -81,7 +81,7 @@ object SerializationStability {
    }
  }

-  // Generated on 20180605-18:45:47 with Scala version 2.13.0-20180604-234247-8dd6ca5)
+  // Generated on 20180701-21:01:46 with Scala version 2.13.0-20180701-205044-6e3f96b)


This is safe to regenerate at the moment. All collection classes have new SerialVersionUIDs and are serialization incompatible with M3 and earlier versions anyway.

Move ClassTagIterableFactory.iterate up to EvidenceIterableFactory.

mslinn · 2019-07-09T00:46:05Z

I find it difficult to understand how to use Iterator.unfold, and the Scaladoc is somewhat oblique. I found an SO posting that helped, but the accompanying explanation is terse. Here are the code examples from the SO posting:

val lines1: Try[Seq[String]] =
  Using(new BufferedReader(new FileReader("file.txt"))) { reader =>
    Iterator.unfold(())(_ => Option(reader.readLine()).map(_ -> ())).toList
  }

val lines2: Seq[String] =
  Using.resource(new BufferedReader(new FileReader("file.txt"))) { reader =>
    Iterator.unfold(())(_ => Option(reader.readLine()).map(_ -> ())).toList
  }

I wonder if it might be helpful to show an unfold equivalent for Scala 2.12?

NthPortal · 2019-07-09T01:19:22Z

I don't disagree that it's oblique. Unfortunately, I'm not sure how to word and explain it better.

To channel my inner Seth, a PR improving the clarity of the docs would probably be accepted. I would do it myself if I had any ideas.

mslinn · 2019-07-09T01:27:38Z

It might be helpful to know a bit about how this method came to be. The use case which inspired the idea, how the original user(s) used this method, the types of tasks that it is particularly well suited, etc. Where did this gem come from?

NthPortal · 2019-07-09T01:48:55Z

it was added to LazyList initially in 1049031

mslinn · 2019-07-09T02:07:02Z

Looks like @julienrf did the work. @julienrf, would you be willing to type out information in response to the questions I asked?

It might be helpful to know a bit about how this method came to be. The use case which inspired the idea, how the original user(s) used this method, the types of tasks that it is particularly well suited, etc. Where did this gem come from?

SethTisue · 2019-07-09T02:29:02Z

In teaching, I would start by teaching iterate, in which we repeatedly feed the output of a function back into the function, collecting the results as we go: Iterator.iterate(0)(_ + 1) generates Iterator(0, 1, 2, 3, ...).

Then, unfold generalizes that in two ways at once:

Instead of always iterating indefinitely, we can indicate when we're done by returning a None instead of a Some
The value we emit to be included in the result is separate from the value that we pass to the next round of iteration.

An example I like is:

scala 2.13.0> def segments[T](xs: List[T]): List[List[T]] =
            |   List.unfold(xs)(xs =>
            |     if (xs.isEmpty) None
            |     else Some(xs.span(_ == xs.head)))
segments: [T](xs: List[T])List[List[T]]

scala 2.13.0> segments(List(1,1,2,3,3,3,4,5,5))
res0: List[List[Int]] = List(List(1, 1), List(2), List(3, 3, 3), List(4), List(5, 5))

Another example is good old Fibonacci:

scala 2.13.0> LazyList.unfold((1, 1)){case (a, b) => Some((a, (b, a + b)))}
res5: scala.collection.immutable.LazyList[Int] = LazyList(<not computed>)

scala 2.13.0> res5.take(10).force
res6: scala.collection.immutable.LazyList[Int] = LazyList(1, 1, 2, 3, 5, 8, 13, 21, 34, 55)

At each stage of the loop, we need to have kept track of two Ints in order to compute the next element, but we only want to actually emit one of them into the result.

mslinn · 2019-07-09T02:45:34Z

@SethTisue Thanks for the quick response. You addressed "how", which is helpful. I'd also like to know "why" and "when". How does this method compare with alternatives? When is it better, and why? What types of problems might this method be well suited for?

Also I am unclear about how results accumulate.

NthPortal · 2019-07-09T06:14:27Z

@mslinn they accumulate results effectively by calling from(Iterator.unfold(start)(op))

julienrf · 2019-07-09T08:32:25Z

@mslinn

unfold is to producing collections as fold is to consuming collections. It provides a purely functional way of producing a collection of which we don’t know the size and elements a priori (we compute the elements one after the other, until we compute a None termination signal, or possibly infinitely). I don’t think we have benchmarks about it but I doubt it has a better performance than a while loop and a Builder, which is the imperative analogous of unfold.

It is more powerful than apply (e.g. List(1, 1, 2, 3)), which constrains the user to enumerate all the elements of the collection. It is more powerful than tabulate, which constrains the user to provide the collection length and is limited to collections whose elements can be computed from their index. It is also more powerful than iterate, which is limited to collections whose nth element can be computed from the n-1th element.

We can prove that by implementing iterate, tabulate, and apply in terms of unfold.

def apply[A](as: A*): List[A] =
  List.unfold(as) { case h +: t => Some((h, t)) case _ => None }

def tabulate[A](length: Int)(f: Int => A): List[A] =
  List.unfold(length)(n => if (n > 0) Some((f(n), n - 1)) else None)

def iterate[A](start: A, length: Int)(f: A => A): List[A] =
  List.unfold((length, start)) {
    case (0, _) => None
    case (n, a) => Some((a, (n - 1, f(a))))
  }

Following the principle of least powerful abstraction, you should use unfold when none of apply, tabulate or iterate is powerful enough for you.

Ichoran · 2019-07-09T15:26:41Z

This is great reasoning and I like the explanation, but note that iterate is (approximately) equally powerful when applied to a lazy collection because

def unfold[A, S](start: S)(op: S => Option[(A, S)]): List[A] =
  Iterator.
    iterate(op(start))(_.flatMap{ case (_, s) => op(s) }).
    map(_.map(_._1)).
    takeWhile(_.isDefined).
    flatten.
    toList

Thus, one should unfold not when iterate isn't powerful enough (it always is), but when it is a better match to your meaning (i.e. you create some hidden state that is iterated, from which you can produce your values).

mslinn · 2019-07-09T17:23:47Z

It is helpful to see unfold expressed in terms of Iterator.iterate, thanks.

Why does unfold use List.from instead of List.apply or calling toList on the Iterator.iterate results? The List.from docs don't offer any reasons for when that method should be preferred.

julienrf · 2019-07-09T17:30:11Z

List.apply takes a Seq as parameter, not an Iterator. Calling .toList or List.from are equivalent.

Ichoran · 2019-07-09T19:34:24Z

@mslinn - No particular reason. List.from made more sense (stylistically) in the first draft I wrote, but after refactoring it doesn't look as nice as .toList, so I've switched it.

NthPortal requested a review from julienrf June 24, 2018 04:33

scala-jenkins added this to the 2.13.0-M5 milestone Jun 24, 2018

Ichoran suggested changes Jun 24, 2018

View reviewed changes

Ichoran approved these changes Jun 24, 2018

View reviewed changes

NthPortal commented Jun 24, 2018

View reviewed changes

NthPortal changed the title ~~bug#10955 Add Iterator.unfold~~ bug#10955 Add Iterator.unfold and Iterable.unfold Jun 24, 2018

NthPortal force-pushed the bug#10955/PR branch from 1eaffd5 to fdeacf4 Compare June 24, 2018 22:06

julienrf reviewed Jun 25, 2018

View reviewed changes

NthPortal force-pushed the bug#10955/PR branch from fdeacf4 to 63dd89a Compare June 28, 2018 04:30

NthPortal changed the title ~~bug#10955 Add Iterator.unfold and Iterable.unfold~~ bug#10955 Add Iterator.unfold and IterableFactory.unfold Jun 28, 2018

NthPortal changed the title ~~bug#10955 Add Iterator.unfold and IterableFactory.unfold~~ Add Iterator.unfold and IterableFactory.unfold Jun 28, 2018

NthPortal force-pushed the bug#10955/PR branch from 63dd89a to 55d550a Compare June 28, 2018 04:47

NthPortal force-pushed the bug#10955/PR branch from 6e3f96b to e908c1c Compare July 1, 2018 21:02

lrytz assigned julienrf Jul 2, 2018

julienrf reviewed Jul 2, 2018

View reviewed changes

NthPortal mentioned this pull request Jul 4, 2018

Lazier LazyList. #6880

Closed

szeiger reviewed Jul 6, 2018

View reviewed changes

NthPortal added 2 commits July 7, 2018 02:38

bug#10955 Add Iterator.unfold and IterableFactory.unfold

b4f9deb

Move ClassTagIterableFactory.iterate up

8e36376

Move ClassTagIterableFactory.iterate up to EvidenceIterableFactory.

NthPortal force-pushed the bug#10955/PR branch from 7c3f464 to 8e36376 Compare July 7, 2018 06:40

julienrf merged commit f34a1e1 into scala:2.13.x Jul 9, 2018

NthPortal deleted the bug#10955/PR branch July 9, 2018 12:59

SethTisue added the release-notes worth highlighting in next release notes label Jul 9, 2018

tgrospic mentioned this pull request Aug 2, 2021

Add transaction api rchain/rchain#3433

Merged

5 tasks

Add Iterator.unfold and IterableFactory.unfold #6851

Add Iterator.unfold and IterableFactory.unfold #6851

Conversation

NthPortal commented Jun 24, 2018

Ichoran left a 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

NthPortal Jun 24, 2018 • edited Loading

Choose a reason for hiding this comment

Ichoran Jun 24, 2018 • edited Loading

Choose a reason for hiding this comment

Ichoran commented Jun 24, 2018

Choose a reason for hiding this comment

tarsa Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NthPortal Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

julienrf commented Jun 25, 2018

julienrf commented Jun 28, 2018 via email

NthPortal commented Jul 1, 2018

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mslinn commented Jul 9, 2019

NthPortal commented Jul 9, 2019

mslinn commented Jul 9, 2019

NthPortal commented Jul 9, 2019 • edited Loading

mslinn commented Jul 9, 2019 • edited Loading

SethTisue commented Jul 9, 2019 • edited Loading

mslinn commented Jul 9, 2019 • edited Loading

NthPortal commented Jul 9, 2019 • edited Loading

julienrf commented Jul 9, 2019 • edited Loading

Ichoran commented Jul 9, 2019 • edited Loading

mslinn commented Jul 9, 2019 • edited Loading

julienrf commented Jul 9, 2019

Ichoran commented Jul 9, 2019 • edited Loading

NthPortal Jun 24, 2018 •

edited

Loading

Ichoran Jun 24, 2018 •

edited

Loading

tarsa Jun 25, 2018 •

edited

Loading

NthPortal Jun 28, 2018 •

edited

Loading

NthPortal commented Jul 9, 2019 •

edited

Loading

mslinn commented Jul 9, 2019 •

edited

Loading

SethTisue commented Jul 9, 2019 •

edited

Loading

mslinn commented Jul 9, 2019 •

edited

Loading

NthPortal commented Jul 9, 2019 •

edited

Loading

julienrf commented Jul 9, 2019 •

edited

Loading

Ichoran commented Jul 9, 2019 •

edited

Loading

mslinn commented Jul 9, 2019 •

edited

Loading

Ichoran commented Jul 9, 2019 •

edited

Loading