[Issue 28415] Add unique!(f, itr) #28737
Conversation
base/set.jl
Outdated
| ``` | ||
| """ | ||
| function unique!(f::Callable, C) | ||
| out = Vector{eltype(C)}() |
|
Nothing actually. It was a vestige of my starting with unique(f:Callable,
C) .... I have pushed a change removing that line.
Thanks for the catch.
rv
…On Sat, Aug 18, 2018 at 2:22 AM laborg ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In base/set.jl
<#28737 (comment)>:
> +"""
+ unique!(f, itr)
+
+In-place replace an array containing one value from `itr` for each unique value produced by `f`
+applied to elements of `itr`.
+
+# Examples
+```jldoctest
+julia> v = Vector([1, -1, 3, -3, 4, -4, 5, -5, 6, -6])
+julia> unique!(x -> x^2, v)
+julia> print(v)
+[1, 3, 4, 5, 6]
+```
+"""
+function unique!(f::Callable, C)
+ out = Vector{eltype(C)}()
What should this out do?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#28737 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AY4jGt5UUmMIDqQngvrx6XZ9sxlzQftiks5uR7KxgaJpZM4WCbjF>
.
--
Sankara Healthcare Foundation - Give the gift of health.
http://www.giftofhealth.us/ <http://www.sankaracancer.org/>
|
…ique(f::Callable, itr) is much faster than the current version. The culprit seems to be splice!
|
I did some performance measurements and reimplemented it using unique() ...
apparently, the many calls to splice! was not helping performance. That's
also pushed.
On Sat, Aug 18, 2018 at 10:06 AM Rajesh Vaidheeswarran <[email protected]>
wrote:
… Nothing actually. It was a vestige of my starting with unique(f:Callable,
C) .... I have pushed a change removing that line.
Thanks for the catch.
rv
On Sat, Aug 18, 2018 at 2:22 AM laborg ***@***.***> wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In base/set.jl
> <#28737 (comment)>:
>
> > +"""
> + unique!(f, itr)
> +
> +In-place replace an array containing one value from `itr` for each unique value produced by `f`
> +applied to elements of `itr`.
> +
> +# Examples
> +```jldoctest
> +julia> v = Vector([1, -1, 3, -3, 4, -4, 5, -5, 6, -6])
> +julia> unique!(x -> x^2, v)
> +julia> print(v)
> +[1, 3, 4, 5, 6]
> +```
> +"""
> +function unique!(f::Callable, C)
> + out = Vector{eltype(C)}()
>
> What should this out do?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#28737 (review)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AY4jGt5UUmMIDqQngvrx6XZ9sxlzQftiks5uR7KxgaJpZM4WCbjF>
> .
>
--
Sankara Healthcare Foundation - Give the gift of health.
http://www.giftofhealth.us/ <http://www.sankaracancer.org/>
--
Sankara Healthcare Foundation - Give the gift of health.
http://www.giftofhealth.us/ <http://www.sankaracancer.org/>
|
|
Typically |
|
I don't care one way or another. I don't see any "dishonesty" in the
implementation though. For my personal uses I did try both a native !
version that i checked in earlier and the one that rides on top of the non!
version that i have checked in now performs much better with just one extra
allocation than the non! version while the native one did not do well at
all. I tested them both repeatedly over many million entry vectors with
quite a few callables.
What is checked in is a legit and a well performing implementation IMO.
Feel free to accept it or reject it as you see fit.
…On Sun, Aug 19, 2018, 05:45 Kristoffer Carlsson ***@***.***> wrote:
Typically ! versions perform better than the non-bang versions because
they don't have to allocate memory for the output. Here, the ! version is
actually slower than the non-bang which to me is a bit "dishonest". If
there is no efficient way of doing it in-place, perhaps it is ok just to
let the user do the copying so it is clear what is actually happening? Just
my thought.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#28737 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AY4jGhP_IMfPKFZpC7xWn2egqzFZC3hbks5uSTOzgaJpZM4WCbjF>
.
|
|
It's not appropriate to label this an in-place function if it makes use of the not-in-place version. This can be done in place, preserving order for the entries that are to appear in the output, with pointer-swapping similar to what is done in the |
|
That's a reasonable critique of the implementation and a reasonable suggestion. The funny thing is that I had implemented that as well initially (even before the first version that I checked in), and I was happy with the lack of complexity in using the not-in-place function. But, I understand philosophically where you come from. I'll push the other implementation. |
base/set.jl
Outdated
| @@ -191,8 +191,7 @@ function unique!(f::Callable, A) | |||
| done = true | |||
| end | |||
| end | |||
| if cur != index splice!(A, index:cur-1); end | |||
| A | |||
| ifelse(cur != index, resize!(A, index-1), A) | |||
There was a problem hiding this comment.
This calls resize! unconditionally.
There was a problem hiding this comment.
Thanks for the catch.
Actually, thinking more about it... that was a rookie julia user mistake on my part in the use of ifelse... however, interestingly, it does not impact the correctness of the function.
if
resize!(array, length(array))
is efficient and does not attempt to resize (which seems to be close to the case... when I attempt a resize on a 2_000_000 element array of {Any,1}, it completed in 0.000006 seconds (5 allocations: 176 bytes)
Then, the actual code could just unconditionally return a
resize!(A, index-1)
instead of testing.
There was a problem hiding this comment.
True. resize! does that check for you anyway, I'm pretty sure.
|
Is there anything more I need to do for this pull to be reviewed and dispositioned? |
I needed to use unique! and had implemented it... but I realized that it was an already open issue. So, just wanted to contribute my change.
If this implementation works for you, please use it.