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 rand(::Tuple) #25278

Merged
merged 2 commits into from Dec 4, 2018

Conversation

4 participants
@rfourquet
Copy link
Contributor

commented Dec 27, 2017

PR based on #24874, only 2 new commits here.

Knowing the length a compile time gives a big advantage to something like rand(('a', 'b')) over rand(['a', 'b']) (many times faster). This PR implements optimizations for tuples up to length 4, but it could easily be extended. The second commit here shows a use case for this, and I also need that occasionally in my programs.

But this raise one question: what to do for rand((1, 2, 3)). Currently, this calls the array contructor. So it would be a bit inconsistent to have rand(::Tuple) give a scalar except when the tuple is of type Dims. I see few possiblities about this:

  • live with this small gotcha
  • make Dims a dedicated types, instead of an alias for tuples of Int. This is a big change, unlikely to happen for 1.0. But I guess most of the time you get a Dims object by calling size on an array, so it doesn't seem to be a no-go in usability.
  • if we go with #24912, we could have either rand(Array, (1, 2, 3)) or rand(1, 2, 3) for the construction of an array, i.e. if the size is specified as a tuple, you get to give Array as a parameter.
  • wait and see if the array versions of rand are disabled, in favor of a Vector constructor (with a Rand iterator).
  • changing rand to require specifying the type of generated value (currently Float64 is the default) (EDITED)

Thoughts?

@Sacha0

This comment has been minimized.

Copy link
Member

commented Dec 27, 2017

Interesting collision! :) Something of an analog to the crux of #16029 (recapped in the "tour of the issues" section's first paragraph in #24595 (comment)), which is one of the primary motivations for moving towards #24595. Best!

@rfourquet

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2017

Thanks for making this connection! Which reminded me of another possibility to avoid this collision: not having a default type (currently Float64) of generated random values.

I think generating a random value from a tuple would be a useful addition, which is not urgent in itself (could be 1.x), but working-around the collision could involve having to make a breaking change now (like e.g. having rand not default to Float64 anymore), so I will mark for triage (with low priority).

@rfourquet rfourquet added the triage label Dec 28, 2017

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented Dec 28, 2017

I would say this is already technically a collision:

julia> rand(1:2, 3)
3-element Array{Int64,1}:
 2
 1
 1

julia> rand(2, 3)
2×3 Array{Float64,2}:
 0.290165  0.0238462  0.180859
 0.705275  0.782572   0.682846

(since 2 is iterable)

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented Dec 28, 2017

I think we'll have to go through a process where rand((m,n)) is replaced before we can add this.

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented Dec 28, 2017

From triage: deprecate rand(tuple) to rand(tuple...) so we can add this in 1.0 (at any point).

@JeffBezanson JeffBezanson removed the triage label Dec 28, 2017

@rfourquet

This comment has been minimized.

Copy link
Contributor Author

commented Dec 29, 2017

deprecate rand(tuple) to rand(tuple...) so we can add this in 1.0 (at any point).

To be clear: only deprecate rand([rng], tuple), or also when the type is specified, like rand([rng], Float64, tuple) ?
The latter doesn't have a collision, e.g with this PR we could even write rand((1, 2, 3), (4, 5)) to have a matrix filled with elements taken from (1, 2, 3).

I would say this is already technically a collision: [...]

deprecating rand(tuple) won't solve this one... shouldn't we deprecate the Float64 default too? It's not clear to me that floats in [1,0) is generally a good default for random numbers. What are the main use cases for it?

@Sacha0

This comment has been minimized.

Copy link
Member

commented Dec 29, 2017

Deprecating both rand(dims::Integers...) and rand(dims::Tuple) is appealing, convenient as rand(shape) and rand(shape...) sometimes are. To my surprise, a brief skim suggests that a solid majority of rand invocations in test/linalg take either an element type or collection as their first argument, for what that's worth. Best!

@rfourquet rfourquet added this to the 1.0 milestone Jan 4, 2018

@rfourquet

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2018

Bump, I need a bit more information to finish this PR (cf. my last comment above). I would also be in favor to deprecate Float64 as the default type for generated values (i.e. requiring specifying S in rand([rng], S, [dims...]).

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

It seems to me we should only deprecate rand(tuple). I would be very sad to lose rand(m, n) and rand(n) though.

@rfourquet rfourquet referenced this pull request Jan 6, 2018

Merged

deprecate rand(::Tuple) #25429

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented Jan 10, 2018

Requires the deprecation to be released first, so moving this to 1.x.

@JeffBezanson JeffBezanson modified the milestones: 1.0, 1.x Jan 10, 2018

@rfourquet

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2018

funny fact: this API conflict was already spotted 3 and a half years ago!

@rfourquet rfourquet force-pushed the rf/rand/fromtuples branch from 4c9000d to 1594a1c Aug 13, 2018

@rfourquet

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2018

This is now rebased and should be good for merging. But I'm not clear on how to update the NEWS file, which has only a v1.0.0 title (not a v1.1.0 one).


### 2

Sampler(RNG::Type{<:AbstractRNG}, t::Tuple{A,B}, n::Repetition) where {A,B} =

This comment has been minimized.

Copy link
@rfourquet

rfourquet Aug 13, 2018

Author Contributor

I think that at one point, this signature was in some cases less efficient than Sampler(::Type{RNG}, t::Tuple{A,B}, n::Repetition) where {RNG<:AbstractRNG,A,B}, can there still be occasional observable performance differences ?

@rfourquet rfourquet force-pushed the rf/rand/fromtuples branch from 1594a1c to c15a86c Aug 13, 2018

@rfourquet rfourquet changed the title [RFC] add rand(::Tuple) add rand(::Tuple) Aug 16, 2018

@rfourquet rfourquet referenced this pull request Aug 16, 2018

Open

add rand(::Pair) #28704

@vtjnash

This comment has been minimized.

Copy link
Member

commented Aug 16, 2018

Should this be map(rand, tuple)?

@rfourquet

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2018

Should this be map(rand, tuple)?

I don't believe so. Here the proposal is to have rand((1, 2)) produce either 1 or 2, which is expected according to the current API, and useful. A future proposal (in the same line as yesterday's #28705) would be rand(Tuple{Int,Int}) to produce a random tuple of Ints of length 2, which could also be implemented as map(rand, (Int, Int)).

@rfourquet rfourquet force-pushed the rf/rand/fromtuples branch from c15a86c to 3ba74d6 Nov 30, 2018

@rfourquet

This comment has been minimized.

Copy link
Contributor Author

commented Nov 30, 2018

News added, I will end up merging this soon (was already triaged).

@rfourquet rfourquet merged commit 5976236 into master Dec 4, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
julia freebsd ci Build done
Details

@rfourquet rfourquet deleted the rf/rand/fromtuples branch Dec 4, 2018

@rfourquet

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2018

There was an (hopefully) unrelated error on Appveyor with the Profile test, if someone is interested in investigating: https://ci.appveyor.com/project/JuliaLang/julia/builds/20680685/job/0krm1ke8dp4dwcfc

fredrikekre added a commit that referenced this pull request Dec 5, 2018

Addition of NEWS and compat admonitions for important
changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.

Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>

fredrikekre added a commit that referenced this pull request Dec 5, 2018

Addition of NEWS and compat admonitions for important
changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.

Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>

fredrikekre added a commit that referenced this pull request Dec 5, 2018

Compat admonitions and NEWS for Julia 1.1 (#30230)
Addition of NEWS and compat admonitions for important changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.


Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.