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.

Sign up for GitHub

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

Jump to bottom

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

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

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

JordanMartinez opened this issue Aug 8, 2022 · 3 comments

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.

@JordanMartinez JordanMartinez mentioned this issue Aug 13, 2022
Closed
4 tasks
@nwolverson
Copy link

nwolverson commented Oct 18, 2022 •
edited
Loading

(\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
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make groupBy use pairwise function JordanMartinez/purescript-arrays
3 participants
@natefaubion @nwolverson @JordanMartinez