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

enable rand(::Union{AbstractSet,Associative}) #22228

Merged
merged 7 commits into from
Jun 13, 2017
Merged

Conversation

rfourquet
Copy link
Member

@rfourquet rfourquet commented Jun 5, 2017

This was previously implemented only for concrete types from base (i.e. Set, IntSet, Dict).
WIP as I don't manage to put some method definitions of GenericSet and GenericDict in test.jl (so I put them in test/random.jl in the meantime), someone has an idea? cf. #22288.

@ararslan ararslan added the randomness Random number generation and the Random stdlib label Jun 5, 2017
test/random.jl Outdated
Base.convert(::Type{$G}, s::$A) = $G(s)
Base.done(s::$G, state) = done(s.s, state)
Base.next(s::$G, state) = next(s.s, state)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

indent's off here

@rfourquet rfourquet mentioned this pull request Jun 6, 2017
@rfourquet
Copy link
Member Author

@stevengj , @ararslan , thanks for your help, I moved to this PR the warning about linear complexity in the general case (as it is solved now for strings in #22224).

base/random.jl Outdated
constant complexity) is available (which is the case for `Dict`,
`Set` and `IntSet`). For more than a few calls, use `rand(rng,
collect(s))` instead (or `rand(rng, Dict(s))`, or `rand(rng,
Set(s))`).
Copy link
Contributor

Choose a reason for hiding this comment

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

the frequent parentheticals are a bit distracting here, the "(which is the case for ...)" is the only one that wouldn't work fine as just a normal part of the sentence

Copy link
Member

Choose a reason for hiding this comment

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

Actually even that would be fine as part of the sentence, IMO.

The complexity of rand(rng, s::Union{Associative,AbstractSet}) is linear in the length of s, unless an optimized method with constant complexity is available, which is the case for Dict, Set, and IntSet. For more than a few calls, use rand(rng, collect(s)) instead, or rand(rng, Dict(s)) or rand(rng, Set(s)) as appropriate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok thank you both, I updated. I also put this note after the examples, which seem more important to me in most of the cases.

@rfourquet rfourquet changed the title [WIP] enable rand(::Union{AbstractSet,Associative}) enable rand(::Union{AbstractSet,Associative}) Jun 8, 2017
@ararslan
Copy link
Member

Looks like this needs a rebase but then should be good to go.

@@ -11,6 +11,8 @@ test set that throws on the first failure. Users can choose to wrap
their tests in (possibly nested) test sets that will store results
and summarize them at the end of the test set with `@testset`.
"""
:Test # cf. #22288
Copy link
Contributor

Choose a reason for hiding this comment

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

guess this commit should be squashed then, on merge if not before

Copy link
Member Author

Choose a reason for hiding this comment

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

I would say all commits can be squashed on merge, and I can squash before if prefered.

@ararslan ararslan merged commit c6799e6 into master Jun 13, 2017
@ararslan ararslan deleted the rf/rand-abstract-setdict branch June 13, 2017 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
randomness Random number generation and the Random stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants