Skip to content

Add Base.allunique#15914

Merged
StefanKarpinski merged 6 commits intoJuliaLang:masterfrom
tpapp:master
Apr 27, 2016
Merged

Add Base.allunique#15914
StefanKarpinski merged 6 commits intoJuliaLang:masterfrom
tpapp:master

Conversation

@tpapp
Copy link
Copy Markdown
Contributor

@tpapp tpapp commented Apr 17, 2016

Allows testing if elements in a collection are distinct (when compared
with isequal). Terminates early in case of repeated elements, has
methods for collections which are by construction distinct. Also added
tests.

See discussion at issue #15803, which this PR fixes.

tpapp added 2 commits April 17, 2016 20:15
Allows testing if elements in a collection are distict (when compared
with isequal). Terminates early in case of repeated elements, has
methods for collections which are by construction distinct. Also added
tests.

See discussion at issue #15803.
The typo also hid the error in the function body that was not caught by
the unit tests, now that is also fixed.
base/set.jl Outdated

alldistinct(::Set) = true

alldistinct(r::Range) = (length(r) == 1) || (step(r) > 0)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be:

(step(r) != 0) || (length(r) <= 1)

or potentially:

alldistinct{T}(r::Range{T}) = (step(r) !== zero(T)) || (length(r) <= one(T))

and add tests for negative step and zero length ranges.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Made the change, with tests.

Suggestion by @hayd. Add tests for negative ranges, and also empty
collections.
base/set.jl Outdated
end

"""
alldistinct(itr)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to be added to rst docs too

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks for the reminder.

@tpapp
Copy link
Copy Markdown
Contributor Author

tpapp commented Apr 23, 2016

Is there anything preventing a merge? I understand the core devs are busy so it might take time, but in case I missed something that prevents a merge that I can fix please let me know.

@tkelman
Copy link
Copy Markdown
Contributor

tkelman commented Apr 23, 2016

I think this is good. If no one has any more feedback, I'll merge soon.

Note for next time, it can help in keeping things organized if you use a new branch (other than master of your fork) for making each pull request.

base/set.jl Outdated
"""
alldistinct(itr)

Test if all values from `itr` are distinct, when compared with `isequal`.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, spoke too soon maybe - technically this isn't testing using isequal until #9381 gets resolved, right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?in says that for sets, isequal is used, so that looks OK.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Test if/Return true if/

No comma before "when".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevengj: thanks, pushed minor change correcting that.
@tkelman: Sorry that I did not branch, I am still learning the ropes, will do it next time.

I hope that this can be merged now, but let me know if I should do anything else.

@tpapp tpapp mentioned this pull request Apr 24, 2016
Following discussion for issue #15803, this emerged as the preferred alternative.
@nalimilan
Copy link
Copy Markdown
Member

Thanks. Can you squash the commits?

@KristofferC
Copy link
Copy Markdown
Member

Can't we start using githubs own squasher?

@stevengj
Copy link
Copy Markdown
Member

Yes, PRs no longer have to squash commits manually, thank goodness.

@stevengj stevengj changed the title Add Base.alldistinct. Add Base.allunique Apr 27, 2016
@StefanKarpinski
Copy link
Copy Markdown
Member

AppVeyor seems to be stuck, but it got to SUCCESS, so I'm calling it a pass.

@StefanKarpinski StefanKarpinski merged commit 2cc803b into JuliaLang:master Apr 27, 2016
@tkelman
Copy link
Copy Markdown
Contributor

tkelman commented Apr 28, 2016

when did this get renamed to allunique? I thought alldistinct was clearer

@StefanKarpinski
Copy link
Copy Markdown
Member

There was a whole conversation about it.

@DNF2
Copy link
Copy Markdown

DNF2 commented Apr 28, 2016

There's a problem here which also popped up in #15009 , namely that you can have 'oversampled' ranges of floats:

>> r = linspace(1.0, nextfloat(1.0), 10)
>> allunique(r)
true
>> allunique(collect(r))
false

As suggested in #15009 (comment) you can prevent this with the following check:

last(r) >= nextfloat(first(r), length(r) - 1)

One more thing: This comparison

length(r) <= one(T)

seems strange to me. length always returns an Int, right? So it should be length(r) <= 1.

(I tried all this on 0.4.5, so apologies if linspace or length have somehow changed in 0.5.)

Oh, also, isn't this a bit pointless: step(r) != zero(T) instead of step(r) != 0?
The compiler figures it out and there is no performance difference. Aren't the zero(T) and one(T) constructs mainly for avoiding type instabilities? No harm, though, I guess.

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 this pull request may close these issues.

8 participants