randstring: allow specifying chars to pick from #22222

rfourquet · 2017-06-05T08:16:04Z

This allows the API randstring([rng], [chars], len=8), e.g. randstring('a':'z') will produce a random string of length 8 of lower case letters.

~~I also added an alias as a method of rand: rand([rng], ::Type{String}, [chars], len=8). I'm not sure both ways should exist, hence requesting for comments.~~ EDIT: this will be for another time (possibly with a different API).

tkelman · 2017-06-05T19:46:46Z

If rand(String, args...) is going to do the same thing as randstring(args...), do we need both?

rfourquet · 2017-06-06T10:14:57Z

If rand(String, args...) is going to do the same thing as randstring(args...), do we need both?

I don't think so, but this is the point of my "RFC". I would tend to think that the rand(String,...) method is enough, as it's easy for those needing the functionality often to create a shorter randstring alias themseves, and it's probably more discoverable as a rand method. On the other hand, this is a different rand API than usual, with an additional len parameter and without the possibility to create an array by passing a dims parameter (I didn't see it be useful, but I may be wrong?), so this may seen as breaking the consistency of the previous API. Now that I think more about it, it may be safer to just keep randstring for now, and think more broadly about the rand-related API: for example I had the idea to try rand([rng], Sparse(type, proba), dims...) as a replacement for sprand(rng, type, dims, proba). Similarly this could be rand([rng], StringType(chars, len), [dims]) instead of randstring (those would be quite consistent with the current rand API). Also I had proposed rand([rng], Bit, dims) instead of bitrand but thist didn't get support.

tkelman · 2017-06-07T11:01:28Z

base/random.jl

+"""
+    randstring([rng=GLOBAL_RNG], [chars::AbstractArray{<:Union{UInt8,Char}}], [len=8])
+
+Create a random ASCII string of length `len`, consisting of characters


is it ASCII if chars has non-ascii in it?

Ah good catch, indeed not, I will update the doc.

rfourquet · 2017-06-07T11:10:41Z

I decided to remove the proposed rand([rng], Type{String}) alias for now. I plan to extend the possible types for chars parameters, in particular to allow passing a string (e.g. randstring("asd")) when #22224 is merged.

tkelman · 2017-06-08T19:02:12Z

base/random.jl

-    randstring(r::AbstractRNG) = randstring(r,8)
-    randstring(n::Int) = randstring(GLOBAL_RNG, n)
-    randstring() = randstring(GLOBAL_RNG)
+    randstring(r::AbstractRNG, chars::AbstractArray{<:Union{UInt8,Char}}=b, n::Int=8) =


allowing UInt8 arrays here is a little odd

(and unrelated to this PR, but anyone else think the default length of 8 is kind of arbitrary? was added without much justification in 399c541)

The choice of Char and UInt8 is because the String constructor accepts arrays of those types, and I thougth that String(v::Vector{UInt8}) was taking ownership of v (according to the docs) so that it would be efficient, but I just did few tests and it seems that this constructor actually copies the data from the vector. Anyway, randstring is still about twice as fast when chars is UInt8 data rather than Char.

(And I also find the default length surprisingly arbitrary, not sure what to do about it though).

the docs might need updating in that respect with the StringVector representation change, not sure

rfourquet · 2017-06-15T16:04:40Z

I updated by not constraining the type of chars: it can be any collection of characters compatible with rand. I could use some help for the docstring (is the !!!note useful, or should it be included in the first part of the docstring?).

tkelman · 2017-06-16T07:46:38Z

base/random.jl

+    `UInt8` (more efficient), provided [`rand`](@ref) can randomly
+    pick characters from it.
+"""
+


no empty line here

Oh right, I add an empty line when justifying with emacs, and often forget to remove it :-/

tkelman · 2017-06-16T07:47:47Z

test/random.jl

+        @test length(randstring(rng..., 20)) == 20
+        @test issubset(randstring(rng...), b)
+        for c = ['a':'z', "qwèrtï", Set(Vector{UInt8}("gcat"))],
+            len = [8, 20]


I'd indent this one level further or include it on the same line as the rest of the for, it looks like an assignment rather than a loop variable if you miss the final comma on the previous line

Makes sense.

no, I meant indent this line one level further, not the contents of the block

OK got it, I didn't know this style

I think this is how the matlab auto indenter would handle line continuations of block openers - one of the few useful things from matlab I miss

rfourquet · 2017-06-26T09:35:39Z

Rebased. I will merge in a few days if no objection.

This reverts commit cdb89ac.

rfourquet · 2017-07-04T15:37:48Z

CI failure seems unrelated: it concerns libgit2, and a similar problem appears in other unrelated PRs.

rfourquet added the randomness Random number generation and the Random stdlib label Jun 5, 2017

rfourquet force-pushed the rf/rand-String branch from 60f54ac to cdb89ac Compare June 5, 2017 08:34

rfourquet changed the title ~~RFC: allow specifying chars to pick from in randstring (and add a rand(::Type{String}) alias?)~~ [RFC] randstring: allow specifying chars to pick from (and add a rand(::Type{String}) alias?) Jun 5, 2017

rfourquet added the strings "Strings!" label Jun 5, 2017

tkelman reviewed Jun 7, 2017

View reviewed changes

rfourquet changed the title ~~[RFC] randstring: allow specifying chars to pick from (and add a rand(::Type{String}) alias?)~~ randstring: allow specifying chars to pick from Jun 8, 2017

tkelman reviewed Jun 8, 2017

View reviewed changes

rfourquet force-pushed the rf/rand-String branch from e85434e to 1ff58f5 Compare June 15, 2017 15:32

tkelman reviewed Jun 16, 2017

View reviewed changes

rfourquet force-pushed the rf/rand-String branch from cf4a2e1 to 684d897 Compare June 26, 2017 09:35

rfourquet added 13 commits July 4, 2017 12:27

randstring: simpler, faster

02c5840

randstring can produce strings with other character sets

0e391ad

add rand([rng], ::String, [chars], n=8)

1875d8e

Revert "add rand([rng], ::String, [chars], n=8)"

36461dc

This reverts commit cdb89ac.

the string is not necessarily ASCII (tkelman)

6025b3a

remove constraint on the type of chars

71939e5

add examples and update tests

74cfb72

update docstring

dcd79fb

Int -> Integer

5909c65

remove spurious space (tkelman) and move docstring before definitions

7ba4dc1

adjust indentation in for loop (tkelman)

8684a3b

adjust indentation in for loop, take 2 (tkelman)

08ad6e9

add srand in doc

c64c552

rfourquet force-pushed the rf/rand-String branch from 684d897 to c64c552 Compare July 4, 2017 10:44

rfourquet merged commit c4f2737 into master Jul 4, 2017

rfourquet deleted the rf/rand-String branch July 4, 2017 15:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

randstring: allow specifying chars to pick from #22222

randstring: allow specifying chars to pick from #22222

rfourquet commented Jun 5, 2017 •

edited

Loading

tkelman commented Jun 5, 2017

rfourquet commented Jun 6, 2017

tkelman Jun 7, 2017

rfourquet Jun 7, 2017

rfourquet commented Jun 7, 2017

tkelman Jun 8, 2017

rfourquet Jun 9, 2017

tkelman Jun 9, 2017 •

edited

Loading

rfourquet commented Jun 15, 2017

tkelman Jun 16, 2017

rfourquet Jun 16, 2017

tkelman Jun 16, 2017

rfourquet Jun 16, 2017

tkelman Jun 17, 2017

rfourquet Jun 17, 2017

tkelman Jun 17, 2017

rfourquet commented Jun 26, 2017

rfourquet commented Jul 4, 2017

randstring: allow specifying chars to pick from #22222

randstring: allow specifying chars to pick from #22222

Conversation

rfourquet commented Jun 5, 2017 • edited Loading

tkelman commented Jun 5, 2017

rfourquet commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfourquet commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman Jun 9, 2017 • edited Loading

Choose a reason for hiding this comment

rfourquet commented Jun 15, 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

rfourquet commented Jun 26, 2017

rfourquet commented Jul 4, 2017

rfourquet commented Jun 5, 2017 •

edited

Loading

tkelman Jun 9, 2017 •

edited

Loading