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

groupBy has unexpected l argument for a -> a -> Boolean predicate #229

Open
JordanMartinez opened this issue Aug 8, 2022 · 3 comments
Open

Comments

@JordanMartinez
Copy link
Contributor

See https://try.purescript.org/?gist=26968458c1261226b33e574128ba9aae

For a given array like [0, 1, 2], I expect the l and r arguments to be

  • first run: 0 and 1
  • second run: 1 and 2

However, in the second run, l is 0 rather than 1.

I'd argue that the behavior here is unexpected.

@natefaubion
Copy link

This comes down to the definition of "equivalence relation". (\a b -> a + 1 == b) is a useful equivalence relation, it's just not what the implementation expects. It would be nice if this worked as grouping consecutive elements is a fairly common thing to do.

@nwolverson
Copy link

nwolverson commented Oct 18, 2022

(\a b -> a + 1 == b) is not an equivalence relation, as it is not reflexive, symmetric, or transitive. Obviously the closure of that relation would be, but that would not be useful

It may be a useful relation, but groupBy does specify an equivalence relation.

(To be clear the proposed change to operate only on successive pairs does not change behaviour if the relation is an equivalence relation and I'm happy with that change in principle)

@JordanMartinez
Copy link
Contributor Author

To be clear the proposed change to operate only on successive pairs does not change behaviour if the relation is an equivalence relation and I'm happy with that change in principle

So, should the PR I have open for this #230 be accepted? Or should a new function be defined that has #230's implementation?

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 a pull request may close this issue.

3 participants