add nubOrd and friends #515
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Could you add QuickCheck properties?
…On Jan 25, 2018 4:59 PM, "gbaz" ***@***.***> wrote:
resolves #439 <#439>
I think I have the asymptotics for nubInt right, but a sanity check
wouldn't hurt...
------------------------------
You can view, comment on, or merge this pull request online at:
#515
Commit Summary
- add ordnub and friends
File Changes
- *A* Data/Containers/ListUtils.hs
<https://github.com/haskell/containers/pull/515/files#diff-0> (65)
- *M* containers.cabal
<https://github.com/haskell/containers/pull/515/files#diff-1> (1)
Patch Links:
- https://github.com/haskell/containers/pull/515.patch
- https://github.com/haskell/containers/pull/515.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#515>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABzi_Y9w0DYRVp-ipEmaciQB7QoyUEqtks5tOPkvgaJpZM4RtjZB>
.
|
treeowl
requested changes
Jan 25, 2018
Data/Containers/ListUtils.hs
Outdated
| go s (x:xs) = if x `Set.member` s then go s xs | ||
| else x : go (Set.insert x s) xs | ||
|
|
||
| -- | The `nubOrdOn` function behaves just like `nubOrd` except it preforms comparisons not on the |
Data/Containers/ListUtils.hs
Outdated
| else x : go (IntSet.insert x s) xs | ||
|
|
||
| -- | The `nubIntOn` function behaves just like 'nubInt' except it preforms comparisons not on the | ||
| -- original datatype, but a user-specified projection from that datatype to 'Int'. |
Data/Containers/ListUtils.hs
Outdated
| -- | The `nubIntOn` function behaves just like 'nubInt' except it preforms comparisons not on the | ||
| -- original datatype, but a user-specified projection from that datatype to 'Int'. | ||
| nubIntOn :: (a -> Int) -> [a] -> [a] | ||
| nubIntOn f l = go IntSet.empty l |
There was a problem hiding this comment.
Eta reduce. And it's probably slightly faster to make f an argument to go, here and above.
|
Actually, I changed my mind. We want to let |
|
However however, inlining |
|
Added tests, fixed typo, eta reduced. I did not add fusion. I took a run at it and it wasn't straightforward. Doing a foldr doesn't really work because it reverses the direction in which things are dropped. So that means making it a good consumer is hard. Further, the way in which we end up building out of the fold (which involves taking a snd of the result, since we need to pass around a pair) is rather ugly and I think gets in the way of making it a good producer. |
|
You can definitely make it a good consumer nicely.
nubOrd xs = foldr go stop xs S.empty
…On Jan 26, 2018 1:28 AM, "gbaz" ***@***.***> wrote:
Added tests, fixed typo, eta reduced.
I did not add fusion. I took a run at it and it wasn't straightforward.
Doing a foldr doesn't really work because it reverses the direction in
which things are dropped. So that means making it a good consumer is hard.
Further, the way in which we end up building out of the fold (which
involves taking a snd of the result, since we need to pass around a pair)
is rather ugly and I think gets in the way of making it a good producer.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#515 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABzi_Rdx12RCeiDDrd-_uulbFEWrw1Vdks5tOXCIgaJpZM4RtjZB>
.
|
|
I don't know what that means. In any case, I tried, and didn't get it working. If you know how to get it working, I suggest we merge first, and then you make a further patch. |
|
Sure.
On Jan 26, 2018 1:46 AM, "gbaz" <[email protected]> wrote:
I don't know what that means. In any case, I tried, and didn't get it
working. If you know how to get it working, I suggest we merge first, and
then you make a further patch.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#515 (comment)>,
or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABzi_fEObtSQzcDG-2j09S4MPKfXMb5iks5tOXTPgaJpZM4RtjZB>
.
|
|
Ok, is this good to merge then? |
|
Thanks. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
resolves #439
I think I have the asymptotics for
nubIntright, but a sanity check wouldn't hurt...