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

Add sliding function #87

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

JordyMoos
Copy link

@JordyMoos JordyMoos commented Dec 21, 2017

Hello,

I added the sliding function like the one in scala list

Sliding lets you group a list based on a sliding windows. You can set a size which is the group size. And a step which is how many steps you take for each group. I used this function in scala a lot and i now needed in in elm. So hereby my contribution.

    sliding 1 2 [1, 2, 3, 4, 5] = [[1], [3], [5]]
    sliding 3 1 [1, 2, 3, 4, 5] = [[1, 2, 3], [2, 3, 4], [3, 4, 5]]

Please let me know your thoughts and hopefully it will get accepted.

Cheers

@@ -1218,7 +1255,7 @@ The equality test should be an [equivalence relation](https://en.wikipedia.org/w
- Symmetry - Testing two objects should give the same result regardless of the order they are passed.
- Transitivity - If the test on a first object and a second object results in `True`, and further if the test on that second object and a third also results in `True`, then the test should result in `True` when the first and third objects are passed.

For non-equivalent relations `groupWhile` has non-intuitive behavior. For example, inequality comparisons like `(<)` are not equivalence relations, so do _not_ write `groupWhile (<) [1,3,5,2,4]`, as it will give an unexpected answer.
For non-equivalent relations `groupWhile` has non-intuitive behavior. For example, inequality comparisons like `(<)` are not equivalence relations, so do *not* write `groupWhile (<) [1,3,5,2,4]`, as it will give an unexpected answer.
Copy link
Author

Choose a reason for hiding this comment

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

I think elm format did this. Do you need it to be reverted?

@pzp1997
Copy link
Contributor

pzp1997 commented Dec 21, 2017

Looks very similar (if not identical) to greedyGroupsOfWithStep. Is there some difference that I'm missing? If so, would it be possible to resolve the difference in behavior without adding another variant of the function?

P.S. Either way, I like your name better and I'm in favor of renaming the groupsOf* functions anyway.

@JordyMoos
Copy link
Author

I think you are right! It is already in the library, i overlooked for it. Then this PR wont be needed.

Cheers!

@JordyMoos
Copy link
Author

greedyGroupsOfWithStep and sliding are apparently not behaving the same. sliding stops when the last value of the initial list is used. While is seems that greedyGroupsOfWithStep stops whenever the last value of the initial list is used as the start of the group.

I wrote sliding in elm to have the same behavior as sliding in scala.

Guess you can argue for both, while i favor sliding because you do not end up with smaller groups if you already used those numbers in previous groups.

I made a little demo on ellie to demonstrate the differences.
https://ellie-app.com/s33QBCTvSa1/0

Please let me know your thoughts on this one

@zwilias
Copy link
Member

zwilias commented Dec 22, 2017

Ah, so it's what is currently called groupsOfWithStep.

Perhaps that section of the docs could use a little overview of what the differences between all these functions are. Worse, the docs for greedyGroupsOfWithStep explain what makes it "greedy", and even explain that there is also a non-greedy version.

All that leads me to believe that this section has bad docs: in order to know which of the 5 grouping functions you need, you need to read the docs for all 5 of them, and compare their behaviour in your head and think "does this match my problem". While reading every section, one also needs to side-scroll the examples because only the input shows up, so by the time I can actually see the output, the input is scrolled away.

It's easier said than done, but I think the docs in that section could use some love.

@JordyMoos
Copy link
Author

@zwilias i tested groupsOfWithStep and that one indeed behaves the same. And therefor we do not need this change.

I do agree with the naming/docs issue. But if you have a lot of grouping functions then i will get hard to name them in a way that most people understand or recognize.

@zwilias
Copy link
Member

zwilias commented Dec 22, 2017

Sure. Since the point of this package is to provide a whole bunch of functions for convenience, and I'm sure someone has had a use for every one of those grouping functions, I think finding better names isn't quite as important here as providing very very clear docs.


Split to groups of given size

There is bunch of ways of splitting a list into sublists, so we have different functions for different use-cases.

Slicing and successive groups

There are 3 ways to do this.

groupsOf and greedyGroupsOf of have the same signature but behave differently with "trailing" elements. groupsOf ensure each sublist has the same length and drops elements at the end that don't fit into a group on their own, while greedyGroups will append a shorter list with the leftovers.

groupsOfVarying allows you to create groups of varying length by passing a list with the size of every group.

Grouping with more control and sliding windows

groupsOfWithStep and greedyGroupsOfWithStep both allow passing a "step" parameter, which determines how many elements there are between start of successive groups. They differ in how they handle trailing elements - greedyGroupsOfWithStep will append a shorter list with those leftovers while groupsOfWithStep will drop them.


If each of those functions has a very short and quick recap of its own behaviour, and a couple of short examples with some whitespace for readability, I think this could actually be a pretty manageable section. My descriptions above are too long, but for a first draft, that's okay.

We could even have some gifs/visualizations; helping folks see what a function does would be really helpful.

@pzp1997
Copy link
Contributor

pzp1997 commented Dec 22, 2017

Actually, groupsOfWithStep and sliding are NOT the same, as can be seen in https://ellie-app.com/s33QBCTvSa1/0. The bad inputs are 3 3 [1,2,3,4] and 3 3 [1,2,3,4,5]. I'm pretty sure sliding is actually a combination of the greedy and non-greedy varieties. I haven't thought this through fully, but I think it's equivalent to using groupsOfWithStep when step evenly divides the length of the list and greedyGroupsOfWithStep in all other cases.

@JordyMoos
Copy link
Author

@pzp1997 You are right.

I checked with scala sliding again and it looks like this elm sliding is behaving the same as scala:
https://scastie.scala-lang.org/Wvc2zN2OSXyON4LETe8s3Q

What slidings intention looks like is that it need to cover all values. And it stops when the last value is used. But greedyGroupsOfWithStep continues for an additional round.

greedyGroupsOfWithStep 2 1 [1, 2, 3, 4, 5] == [[1,2],[2,3],[3,4],[4,5],[5]]
sliding 2 1 [1, 2, 3, 4, 5] == [[1,2],[2,3],[3,4],[4,5]]

Sliding uses all values even if it can not fill a group. While groupsOfWithStep does not use the last values if it can not fill a group

groupsOfWithStep 3 3 [1, 2, 3, 4, 5] == [[1,2,3]]
sliding 3 3 [1, 2, 3, 4, 5] == [[1,2,3],[4,5]]

@Chadtech
Copy link
Collaborator

Chadtech commented Jan 5, 2018

Since they are so similar, should we even include this new sliding function?

@JordyMoos
Copy link
Author

JordyMoos commented Jan 5, 2018

I am in need of this behavior and because it is also exists in another language natively hereby the proposal.

@pzp1997
Copy link
Contributor

pzp1997 commented Jan 7, 2018

In my eyes, the problem is that there are too many possible variants of sliding functions to include them all in the library. (And, unfortunately, I'm not sure which ones are actually useful.) What is your use case that requires this exact variant? Why does the existing groupsOf family of functions not suffice?

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.

4 participants