add rand(::String) #22224

rfourquet · 2017-06-05T13:44:16Z

julia> rand("asd", 3)
3-element Array{Char,1}:
 'd'
 'a'
 'a'

This should work more generally for any iterable (with length) in principle, but I'm hesitant to introduce a method of rand with unconstrained parameters: rand([rng], iter), which would mean that this would be the fallback method anytime rand is called on a type for which it's not defined (so maybe this should wait till some traits for iterators are available).

StefanKarpinski · 2017-06-05T14:31:27Z

test/random.jl

-                   (rand(Int, 100), Int)]
+                   (rand(Int, 100), Int),
+                   ("qwèrtï", Char),
+                   (Base.Test.GenericString("qwèrtï"), Char)]


I would throw in a test for empty strings since that seems like a corner case that could easily break. (I checked this out specifically to test that – it works.)

ararslan · 2017-06-05T18:10:53Z

base/random.jl

+
+rand_iter(rng, s, ::Base.IteratorSize) = throw(ArgumentError("iterator must have a known length"))
+
+function rand_iter(rng::AbstractRNG, s, ::Union{Base.HasShape,Base.HasLength})::eltype(s)


Out of curiosity, why is the return type annotation necessary here? c is the value that will be returned and isn't that already guaranteed to have type eltype(s) (or at least <:eltype(s) if eltype(s) is abstract) given that it's an element of s?

inference probably can't determine that i == pos is guaranteed to be true for at least one value of the enumerate loop

Yes Tony is spot on, and also initially I thought to propose this PR working generally on iterators, and the return type annotation could serve as an additional way to check whether the argument was complying to the iterator interface.

Okay, nice. Thanks for the explanation, both of you.

stevengj · 2017-06-05T21:39:45Z

base/random.jl

+    for (i, c) in enumerate(s)
+        i == pos && return c
+    end
+end


I'm a little skeptical of the rand_iter function and AbstractString support. People using rand with other string types will probably not expect this function to be O(n); it seems preferable to throw a MethodError rather than to use this implementation.

Yes I agree, I had the same hesitation and had planned to update the doc today to warn about the complexity. Then I think that with a proper warning those convenience methods (similarly for AbstractSet and Associative at #22228) are not harmful and can be handy.

You could also do rejection sampling: randomly choose and index from start(s):endof(s) and retry if it's not a valid start of a character – i.e. the same thing you do for String.

Oh great! for some reason, I skimmed too fast on the generic code of isvalid and thought it was O(n).

Actually, I was confused not about isvalid, but about the fact that I thought I needed to compute the length somehow, which is O(n) in the generic method. Thanks again @StefanKarpinski, I updated the code.

ararslan · 2017-06-06T17:53:03Z

base/random.jl

 * a type: the set of values to pick from is then equivalent to `typemin(S):typemax(S)` for
  integers (this is not applicable to [`BigInt`](@ref)), and to ``[0, 1)`` for floating
  point numbers;

 `S` defaults to [`Float64`](@ref).

+Note that the complexity of `rand(rng, s::AbstractString)` is linear


Not really necessary, but this could be a Documenter note block, like

!!! note The complexity of `rand(rng, s::AbstractString)` is linear in the length of `s` if `s` is not of type `String`. If called more than a few times, use `rand(rng, collect(s))` instead.

and maybe rand(rng, collect(s)) or rand(rng, String(s)), just to emphasize that String is fine.

"If called more than a few times" is a bit awkward because of the ambiguous subject of "called". Maybe "For generating many random characters, use ..."

I understand, but your new formulation seems to include also the case were I call rand(rng, s, dims) which already does a collect... WDYT?

Maybe "For multiple calls, consider using ..."

rfourquet · 2017-06-08T09:17:55Z

base/random.jl

+            return s[rand(rng, g)]
+        end
+    end
+end


@StefanKarpinski would you mind having a look at this updated generic implementation, to confirm it complies with AbstractString API?

I think it would be better to do i = rand(rng, g); isvalid(s, i) && return i since various string types are expected to implement that without the try/catch. The try/catch only exists as a last resort fall-back, and I believe it is more targeted and only catches UnicodeError exceptions (if it's not already, it should be).

Ok thanks (I believe the idea being that ~~try/catch~~ an exception is expected to occur, and slow down the computation when the isvalid call would generally be faster). I will update. Note that the generic isvalid catches all exceptions (not only UnicodeError). I'm not familiar with strings, but I can open a corresponding simple PR if it helps.

…pinski)

rfourquet · 2017-06-13T18:03:33Z

I rebased (wile changing only the docstring, including "a string (considered as a collection of characters)"), and removed the try/catch block in favor of isvalid.

stevengj · 2017-06-13T18:21:14Z

base/random.jl

-* an `Associative` or `AbstractSet` object, or
+* an indexable collection (for example `1:n` or `['x','y','z']`),
+* an `Associative` or `AbstractSet` object,
+* a string (considered as a collection of characters),


need "or" at the end of this line (or at the beginning of the next line)

realized a problem

rfourquet · 2017-06-14T12:36:53Z

Does it need a NEWS.md entry?

rfourquet · 2017-06-15T09:04:41Z

Thanks all for your help!

StefanKarpinski reviewed Jun 5, 2017

View reviewed changes

StefanKarpinski approved these changes Jun 5, 2017

View reviewed changes

ararslan reviewed Jun 5, 2017

View reviewed changes

stevengj reviewed Jun 5, 2017

View reviewed changes

stevengj previously approved these changes Jun 6, 2017

View reviewed changes

ararslan reviewed Jun 6, 2017

View reviewed changes

This was referenced Jun 7, 2017

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

Merged

randstring: allow specifying chars to pick from #22222

Merged

rfourquet commented Jun 8, 2017

View reviewed changes

rfourquet added 5 commits June 13, 2017 19:20

add rand(::String)

459a126

test for empty strings (StefanKarpinski) and fix bad usage a GLOBAL_RNG

1deb0c5

warn about linear complexity for AbstractString (stevengj)

5e827e9

use O(1) algo for generic AbstractString (StefanKarpinski)

533dfb3

use isvalid instead of try/catch in generic implementation (StefanKar…

3b97b61

…pinski)

rfourquet force-pushed the rf/rand-from-string branch from e2cefe4 to 3b97b61 Compare June 13, 2017 18:01

stevengj reviewed Jun 13, 2017

View reviewed changes

stevengj approved these changes Jun 13, 2017

View reviewed changes

add a missing "or" in doc (stevengj)

fa51ee6

ararslan approved these changes Jun 13, 2017

View reviewed changes

StefanKarpinski approved these changes Jun 13, 2017

View reviewed changes

add NEWS.md entry

a5289a1

StefanKarpinski approved these changes Jun 14, 2017

View reviewed changes

rfourquet merged commit 1c40bab into master Jun 15, 2017

rfourquet deleted the rf/rand-from-string branch June 15, 2017 09:07

quinnj pushed a commit that referenced this pull request Jun 20, 2017

add rand(::String) (#22224)

b924d0d

rfourquet added the randomness Random number generation and the Random stdlib label Jul 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add rand(::String) #22224

add rand(::String) #22224

rfourquet commented Jun 5, 2017

StefanKarpinski Jun 5, 2017

ararslan Jun 5, 2017

tkelman Jun 5, 2017

rfourquet Jun 6, 2017

ararslan Jun 6, 2017

stevengj Jun 5, 2017

rfourquet Jun 6, 2017

StefanKarpinski Jun 6, 2017

rfourquet Jun 6, 2017

rfourquet Jun 7, 2017

ararslan Jun 6, 2017

stevengj Jun 6, 2017

stevengj Jun 6, 2017

rfourquet Jun 6, 2017

stevengj Jun 6, 2017

rfourquet Jun 8, 2017

StefanKarpinski Jun 13, 2017

rfourquet Jun 13, 2017 •

edited

Loading

rfourquet commented Jun 13, 2017

stevengj Jun 13, 2017 •

edited

Loading

rfourquet commented Jun 14, 2017

rfourquet commented Jun 15, 2017


		rand_iter(rng, s, ::Base.IteratorSize) = throw(ArgumentError("iterator must have a known length"))

		function rand_iter(rng::AbstractRNG, s, ::Union{Base.HasShape,Base.HasLength})::eltype(s)

add rand(::String) #22224

add rand(::String) #22224

Conversation

rfourquet commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfourquet Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

rfourquet commented Jun 13, 2017

stevengj Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

rfourquet commented Jun 14, 2017

rfourquet commented Jun 15, 2017

rfourquet Jun 13, 2017 •

edited

Loading

stevengj Jun 13, 2017 •

edited

Loading