Skip to content

.slidingN using tuples - #3727

Merged
larsrh merged 12 commits into
typelevel:masterfrom
Slakah:sliding-n
Feb 7, 2021
Merged

.slidingN using tuples#3727
larsrh merged 12 commits into
typelevel:masterfrom
Slakah:sliding-n

Conversation

@Slakah

@Slakah Slakah commented Jan 2, 2021

Copy link
Copy Markdown
Contributor

This is a kind reminder to run sbt +prePR and commit the changed files, if any, before submitting.

Discussed a little on gitter, this is basically a tupleified version of scala sliding.

These functions differs in that when the sliding size is larger than the size of the list then an empty list is returned, rather than a partial entry.

scala> (1 to 10).toList.sliding(2).toList
val res0: List[List[Int]] = List(List(1, 2), List(2, 3), List(3, 4), List(4, 5), List(5, 6), List(6, 7), List(7, 8), List(8, 9), List(9, 10))

scala> (1 to 10).toList.sliding(4).toList
val res1: List[List[Int]] = List(List(1, 2, 3, 4), List(2, 3, 4, 5), List(3, 4, 5, 6), List(4, 5, 6, 7), List(5, 6, 7, 8), List(6, 7, 8, 9), List(7, 8, 9, 10))

scala> Foldable[List].sliding2((1 to 10).toList)
val res2: List[(Int, Int)] = List((1,2), (2,3), (3,4), (4,5), (5,6), (6,7), (7,8), (8,9), (9,10))

scala> Foldable[List].sliding4((1 to 10).toList)
val res3: List[(Int, Int, Int, Int)] = List((1,2,3,4), (2,3,4,5), (3,4,5,6), (4,5,6,7), (5,6,7,8), (6,7,8,9), (7,8,9,10))

// partial result returned in scala stdlib version

scala> (1 to 2).toList.sliding(4).toList
val res4: List[List[Int]] = List(List(1, 2))

scala> Foldable[List].sliding4((1 to 2).toList)
val res5: List[(Int, Int, Int, Int)] = List()

TODO

  • Tests
  • Generated trait should have a name other than Arity
  • Sensible scaladoc
  • Syntax

@Slakah Slakah changed the title Sliding n .slidingN using tuples Jan 2, 2021
@codecov-io

codecov-io commented Jan 3, 2021

Copy link
Copy Markdown

Codecov Report

Merging #3727 (305d5ba) into master (f6ee812) will decrease coverage by 0.21%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #3727      +/-   ##
==========================================
- Coverage   90.24%   90.02%   -0.22%     
==========================================
  Files         390      390              
  Lines        8865     8886      +21     
  Branches      255      261       +6     
==========================================
  Hits         8000     8000              
- Misses        865      886      +21     

@Slakah
Slakah marked this pull request as ready for review January 3, 2021 23:07
@Slakah

Slakah commented Jan 3, 2021

Copy link
Copy Markdown
Contributor Author

Looks good enough to try and solicit feedback. The one thing which sticks out to me is the fact that the functions can return an empty list when the size of the foldable is less than the size of the window. I personally thing this is sensible, as the sliding function is not useful in these situations.

Discussion/feedback welcome though!

@rossabaker

Copy link
Copy Markdown
Member

I think this is neat. It deserves a better review, but quick response:

The one thing which sticks out to me is the fact that the functions can return an empty list when the size of the foldable is less than the size of the window. I personally thing this is sensible, as the sliding function is not useful in these situations.

That's inconsistent with standard library's sliding, but I don't see what else you can do here other than document it, which you did. It's a fundamental tradeoff of having fixed-size windows.

@johnynek

Copy link
Copy Markdown
Contributor

This is cool.

One thing I wonder about is, rather than materializing a list, what about a fold?

def sliding2FoldLeft[B](init: B)(fn: (B, (A, A)) => B): B

Then you can recover the list by { (tail, head) => head :: tail }.

But in many cases, you might not need to materialize everything, so could go ahead and tear it down there.

@Slakah

Slakah commented Jan 29, 2021

Copy link
Copy Markdown
Contributor Author

Thanks for the input @johnynek , the main motivation of this change was to be a more typesafe List#sliding(n) when you want to do things with the sliding window at certain indexes. So having a type which shows the sliding window is of fixed size is pretty useful, for pattern matching and the like.

re: sliding2FoldLeft, was always a touch nervous about having a concrete type for the list, was considering doing something like

def sliding2M[M[_]: Monoid]: M[(A, A)]

But maybe sliding2FoldLeft makes more sense for the general case. I still like .sliding2 which returns a list, just because of the terseness of the whole thing. I tended to use .sliding(n) quite a bit during Advent of Code, so thought a more typesafe version might be more ergonomic.

@larsrh

larsrh commented Feb 7, 2021

Copy link
Copy Markdown
Contributor

Test failure was because of a flaky test. This is fixed on master.

@larsrh larsrh added this to the 2.4 milestone Feb 7, 2021

@larsrh larsrh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this looks good. We can improve on this further (as e.g. suggested by @johnynek) in a later PR.

@Slakah Slakah mentioned this pull request Dec 20, 2023
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.

5 participants