Adding Type argument to sprandn #29074
Conversation
| @@ -1488,6 +1489,8 @@ julia> sprandn(2, 2, 0.75) | |||
| """ | |||
| sprandn(r::AbstractRNG, m::Integer, n::Integer, density::AbstractFloat) = sprand(r,m,n,density,randn,Float64) | |||
| sprandn(m::Integer, n::Integer, density::AbstractFloat) = sprandn(GLOBAL_RNG,m,n,density) | |||
| sprandn(r::AbstractRNG, ::Type{T}, m::Integer, n::Integer, density::AbstractFloat) where {T} = sprand(r, T, m, n, density) | |||
There was a problem hiding this comment.
Might this definition end up drawing from a uniform rather than normal distribution?
There was a problem hiding this comment.
True! I’ll add tests for the matrix case and fix this
Welcome @lostella! :D |
| @@ -1488,6 +1489,8 @@ julia> sprandn(2, 2, 0.75) | |||
| """ | |||
| sprandn(r::AbstractRNG, m::Integer, n::Integer, density::AbstractFloat) = sprand(r,m,n,density,randn,Float64) | |||
| sprandn(m::Integer, n::Integer, density::AbstractFloat) = sprandn(GLOBAL_RNG,m,n,density) | |||
| sprandn(r::AbstractRNG, ::Type{T}, m::Integer, n::Integer, density::AbstractFloat) where {T} = sprand(r,m,n,density,randn,T) | |||
| sprandn(::Type{T}, m::Integer, n::Integer, density::AbstractFloat) where {T} = sprandn(GLOBAL_RNG,T,m,n,density) | |||
There was a problem hiding this comment.
I imagine that tests for these signatures are inbound per your preceding comment; commenting in case that idea got lost :).
There was a problem hiding this comment.
Indeed, they’re coming probably later today.
|
I added tests, CI is running. I'm sorry for the multiple commits, is there a way to stop the builds (in travis, circleci) for all but the latest commit? It seems like a waste otherwise. |
No worries! CI should cancel the outdated builds automagically :). |
|
Sure. One note: i limited tests for the |
Most sparse matrix functionality is covered in stdlib/SparseArrays/test/sparse.jl; chances are the tests in the file you add here (sparsematrix.jl) belong somewhere in there :). Best! |
|
@Sacha0 @KristofferC – Do you think anything is missing here? Any idea whether this feature will make it to 1.1? |
|
Any chance we can shepherd this first-time pr over the finish line by the time the release drops, @Sacha0 and @KristofferC? Sorry this fell off the radar, @lostella. |
|
Oh no! My beautiful PR... Well, at least it’s reassuring that I didn’t fall too far from the one that got merged. Or maybe it was exactly identical. Next time! |
|
Sorry about that! Your PR is still greatly appreciated ❤️ |
This is a shot at solving #29018. I added tests for the
Vectorcases, but not for theMatrixcase: these are nowhere to be found forsprandandsprandn. Or am I missing something?In addition to this, I did minor rephrasing to the docstring, in particular to how optional arguments are listed: it makes more sense to me like this.
First PR, go easy on me :-)