Skip to content

Use Ord by default for nub, difference, etc #91

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

Closed
hdgarrood opened this issue Feb 6, 2017 · 6 comments
Closed

Use Ord by default for nub, difference, etc #91

hdgarrood opened this issue Feb 6, 2017 · 6 comments

Comments

@hdgarrood
Copy link
Contributor

Currently we use an Eq constraint for functions like nub and difference. However this gives a poor asymptotic complexity; see https://github.com/nh2/haskell-ordnub.

Of course, there are some types which have Eq but not Ord instances so it makes sense to make Eq versions of these functions available. However, most types do have Ord instances so I think the default version should use Ord. That is, I'd prefer to have nub :: forall a. Ord a => Array a -> Array a and additionally nubEq which is the same except with only an Eq constraint.

This is a little bit difficult with respect to dependencies because we would probably want to depend on sets to do this, but maps (which is a dependency of sets) already depends on arrays. So that's one issue to sort out, if we do agree that we want to do this.

@borsboom
Copy link

nub is such a common source of accidental performance problems in Haskell code, so I'm very much +1 on this. The extra package's implementation of nubOrd doesn't depend on sets or maps.

@matthewleon
Copy link
Contributor

This is a little bit difficult with respect to dependencies because we would probably want to depend on sets to do this, but maps (which is a dependency of sets) already depends on arrays. So that's one issue to sort out, if we do agree that we want to do this.

I think this is another reason in favor of purescript-deprecated/purescript-sets#46

@matthewleon
Copy link
Contributor

On further thought, it's not clear to me that Set is necessary, or beneficial, to do this. We could use Array's own sort, which uses FFI to the JS engine's native Array.sort, and do nub the imperative way. It would avoid the extra dep, and also quite possibly be faster.

@matthewleon
Copy link
Contributor

I've taken a shot at nub here: #128

@hdgarrood
Copy link
Contributor Author

Currently nub preserves order, though, ie nub xs == xs when xs has no duplicates. If I understand correctly your implementation doesn’t have this property. I think it’s a useful property, though, and so I’d rather not lose it.

matthewleon added a commit to matthewleon/purescript-arrays that referenced this issue Jan 23, 2018
addresses purescript#91

O(n*logn)
matthewleon added a commit to matthewleon/purescript-arrays that referenced this issue Jan 23, 2018
addresses purescript#91

O(n*logn)
matthewleon added a commit to matthewleon/purescript-arrays that referenced this issue Jan 23, 2018
addresses purescript#91

O(n*logn)
kritzcreek pushed a commit that referenced this issue May 15, 2018
addresses #91

O(n*logn)
@garyb
Copy link
Member

garyb commented May 22, 2018

Resolved in the 0.12 branch

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

No branches or pull requests

4 participants