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.

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

Add Base.allunique #15914

Merged
merged 6 commits into from
Apr 27, 2016
Merged

Add Base.allunique #15914

merged 6 commits into from
Apr 27, 2016

Conversation

tpapp
Copy link
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.

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.

alldistinct(::Set) = true

alldistinct(r::Range) = (length(r) == 1) || (step(r) > 0)
Copy link
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
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.
@@ -141,6 +141,27 @@ function unique(f::Callable, C)
out
end

"""
alldistinct(itr)
Copy link
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
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
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
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.

"""
alldistinct(itr)

Test if all values from `itr` are distinct, when compared with `isequal`.
Copy link
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
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
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
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
Member

Thanks. Can you squash the commits?

@KristofferC
Copy link
Member

Can't we start using githubs own squasher?

@stevengj
Copy link
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
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
Contributor

tkelman commented Apr 28, 2016

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

@StefanKarpinski
Copy link
Member

There was a whole conversation about it.

@DNF2
Copy link

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