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

More efficient slidingN functions #4067

Merged
merged 3 commits into from
Dec 4, 2021
Merged

Conversation

joroKr21
Copy link
Member

@joroKr21 joroKr21 commented Dec 2, 2021

We piggy-back on toIterable.
This is also an easy way to avoid stack overflow.

Fixes #4066

We piggy-back on `toIterable`.
This is also an easy way to avoid stack overflow.
@armanbilge
Copy link
Member

Can we add the example from #4066 as a test?

@joroKr21
Copy link
Member Author

joroKr21 commented Dec 2, 2021

Can we add the example from #4066 as a test?

Where should I add this? FoldableSuite is kinda weird

@satorg
Copy link
Contributor

satorg commented Dec 2, 2021

Aside question: how come slidingN functions return List[TupleN] rather that F[TupleN] itself?

I mean, if F[_] is e.g. LazyList (which is still Foldable) then it will be enforced although it is not necessary to do for such a method.

@johnynek
Copy link
Contributor

johnynek commented Dec 2, 2021

Foldable[F] is really basically ToList[F] to answer your question @satorg .

There is no methods to create an F[_] with Foldable[F], just take them apart.

johnynek
johnynek previously approved these changes Dec 2, 2021
@johnynek
Copy link
Contributor

johnynek commented Dec 2, 2021

I approved, but I do think adding the test that caused a stack overflow would be good.

@satorg
Copy link
Contributor

satorg commented Dec 3, 2021

I'm just thinking – wouldn't Alternative be a better bet for sliding-like functionality eventually?

This is a question for the future of course – not related to this PR specifically.
This PR is great on its own )

armanbilge
armanbilge previously approved these changes Dec 3, 2021
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix :)

tests/src/test/scala/cats/tests/FoldableSuite.scala Outdated Show resolved Hide resolved
Co-authored-by: Arman Bilge <armanbilge@gmail.com>
@johnynek
Copy link
Contributor

johnynek commented Dec 4, 2021

@satorg I don't see a function like this in cats, but I think we could add it:

trait Alternative[F[_]] ... {
  def fromFoldable[G[_], A](ga: G[A])(implicit G: Foldable[G]): F[A] = {
    @tailrec def loop(lst: List[A], left: F[A]): F[A] =
      lst match {
        case a :: as => appendK(left, a)
        case Nil => left
     }

    loop(G.toList(ga), empty)
  } 
}

which we could override with more efficient implementations as needed (such as going in the reverse direction for building list, or using a List.newBuilder).

@johnynek johnynek merged commit abd6c77 into typelevel:main Dec 4, 2021
@joroKr21 joroKr21 deleted the sliding-n branch December 4, 2021 09:47
@satorg
Copy link
Contributor

satorg commented Dec 5, 2021

@johnynek what I really meant is that having both Foldable and Alternative available for F it could be possible to implement something like this:

def slidingN[F[_]: Alternative : Foldable, A](fa: F[A]): F[(A, ..., A)]

while preserving laziness for those F[_] which support it (i.e. LazyList).

What I was thinking about – there's combineEvalK available via Alternative which pretends to support laziness.
But in fact as for now combineEvalK does not support laziness for lazy collections, unfortunately.
Therefore, lazy-aware implementation would also require a FlatMap instance to shift combineK into "lazy" context:

  def lazySliding3[F[_], A](fa: F[A])(implicit FA: Alternative[F], FF: Foldable[F], FM: FlatMap[F]): F[(A, A, A)] = {
    val it =
      FF.toIterable(fa).iterator.sliding(3).withPartial(false)
        .map(_.toList).map { case a1 :: a2 :: a3 :: Nil => (a1, a2, a3) }

    def go(): F[(A, A, A)] = {
      FA
        .pure(Eval.later {
          if (it.hasNext)
            FA.prependK(it.next(), go())
          else
            FA.empty[(A, A, A)]
        })
        .flatMap(_.value) // this trick shifts all prependK into "lazy" context for lazy collections
    }

    go()
  }

Note: this is a "quick and dirty" implementation. It is not stack-safe for non-lazy collections.

@satorg
Copy link
Contributor

satorg commented Dec 5, 2021

The issue with combineEvalK I described here in Discord:
https://discord.com/channels/632277896739946517/632278570512678923/916869020534317076

I am not really sure it is the issue though – maybe it is an expected behavior )

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

Successfully merging this pull request may close these issues.

SlidingN StackOverflow
4 participants