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

deprecate sprandbool + add new syntax for creating random sparse matrix #16098

Merged
merged 2 commits into from
Apr 30, 2016

Conversation

KristofferC
Copy link
Sponsor Member

@KristofferC KristofferC commented Apr 28, 2016

This mimics the one already existing for rand.

Now:

julia> rand(Float32, 3, 3)
3×3 Array{Float32,2}:
 0.938487  0.200827  0.746491
 0.566157  0.650916  0.466414
 0.408861  0.166284  0.648508

julia> sparsity = 0.5
0.5

julia> sprand(Float32, 3, 3, sparsity)
3×3 sparse matrix with 4 Float32 nonzero entries:
    [2, 1]  =  0.181532
    [3, 2]  =  0.51335
    [1, 3]  =  0.630208
    [2, 3]  =  0.632587

[edit by tkelman: closes #11688]

@KristofferC
Copy link
Sponsor Member Author

Probably makes sense to deprecate sprandbool here too since now it is just:

julia> sprand(Bool, 3, 3, 0.5)
3×3 sparse matrix with 5 Bool nonzero entries:
    [2, 1]  =  false
    [1, 2]  =  false
    [2, 2]  =  false
    [3, 2]  =  true
    [3, 3]  =  true

@KristofferC KristofferC changed the title add new syntax for creating random sparse matrix WIP: add new syntax for creating random sparse matrix Apr 28, 2016
@KristofferC
Copy link
Sponsor Member Author

Actually the above is wrong since false shouldn't be stored.

@KristofferC KristofferC changed the title WIP: add new syntax for creating random sparse matrix deprecate sprandbool + add new syntax for creating random sparse matrix Apr 28, 2016
@@ -356,17 +356,17 @@ function sprand(r::AbstractRNG, n::Integer, p::AbstractFloat, rfn::Function)
SparseVector(n, I, V)
end

sprand{T}(::Type{T}, n::Integer, p::AbstractFloat) = sprand(GLOBAL_RNG, n, p, rand, T)
sprand{T}(n::Integer, p::AbstractFloat, ::Type{T}) = sprand(GLOBAL_RNG, n, p, rand, T)
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Should this one be deprecated as well? It doesn't correspond to the dense case.

@tkelman
Copy link
Contributor

tkelman commented Apr 29, 2016

Needs a doc update for the new sprand syntax with eltype first. It's a little annoying that eltype comes last in the existing version of sprand if you want to provide both an RNG and an eltype. Ideally we could make the whole thing more consistent with the way rand works when you want to provide just one or both (but with the additional density parameter).

@KristofferC
Copy link
Sponsor Member Author

Yes, I will make stuff more consistent between rand and sprand but then there will have to be some deprecations for sprand too

@KristofferC
Copy link
Sponsor Member Author

KristofferC commented Apr 29, 2016

Updated. Some usage examples:

julia> sprand(Bool, 3, 3, 0.3)
3×3 sparse matrix with 2 Bool nonzero entries:
    [3, 1]  =  true
    [1, 3]  =  true

julia> sprand(Base.GLOBAL_RNG, Bool, 3, 3, 0.3)
3×3 sparse matrix with 4 Bool nonzero entries:
    [1, 1]  =  true
    [3, 1]  =  true
    [1, 3]  =  true
    [2, 3]  =  true

julia> sprand(Base.GLOBAL_RNG, Float32, 3, 3, 0.3)
3×3 sparse matrix with 2 Float32 nonzero entries:
    [3, 2]  =  0.954499
    [2, 3]  =  0.00251913

julia> sprand(Float64, 3, 3, 0.3)
3×3 sparse matrix with 3 Float64 nonzero entries:
    [2, 1]  =  0.960525
    [3, 1]  =  0.990573
    [1, 3]  =  0.475681

julia> sprand(Float64, 2, 0.3)
Sparse vector of length 2 with 1 Float64 nonzero entries:
  [1]  =  0.446653

julia> sprand(Bool, 5, 0.3)
Sparse vector of length 5 with 2 Bool nonzero entries:
  [2]  =  true
  [5]  =  true


Create a random length `m` sparse vector or `m` by `n` sparse matrix, in
which the probability of any element being nonzero is independently given by
`p` (and hence the mean density of nonzeros is also exactly `p`). Nonzero
values are sampled from the distribution specified by `rfn`. The uniform
values are sampled from the distribution specified by `rfn` and have the type `type`. The uniform
Copy link
Contributor

Choose a reason for hiding this comment

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

run genstdlib

looks really good, thanks for getting to this before I did

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

wow, you are fast.. just ran it.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

think I need to rerun make for the actual docstring to get updated though..

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah make docs is the more precise way that will also rerun bootstrap to get the updated docstring into the system image

@KristofferC
Copy link
Sponsor Member Author

What is the policy regarding documentation? Should documentation for deprecated methods be removed?

@tkelman
Copy link
Contributor

tkelman commented Apr 29, 2016

I think that's what we've been doing. The deprecation warning will point to the new syntax and the new syntax should be as well or better documented than the old syntax.

@KristofferC
Copy link
Sponsor Member Author

I built up a bit of unnecessary CI queue which could possibly be killed.

@nalimilan
Copy link
Member

I've cancelled the duplicate builds.

@tkelman
Copy link
Contributor

tkelman commented Apr 29, 2016

there are hooks that fail fast on redundant builds automatically, so not a big deal. manually cancelling saves maybe a minute or two tops.

edit: unless they builds have already started, in which case manually cancelling is worthwhile

@nalimilan
Copy link
Member

@tkelman The issue is that the obsolete builds had already started, and AFAIK they aren't cancelled automatically in that case.

@KristofferC
Copy link
Sponsor Member Author

Error is OSX timeout.

@tkelman
Copy link
Contributor

tkelman commented Apr 29, 2016

Last few places it should be cleaned up, from git grep:

base/exports.jl:1462:    sprandbool,
base/sparse.jl:34:    sprand, sprandbool, sprandn, spzeros, symperm, nnz
contrib/BBEditTextWrangler-julia.plist:1073:            <string>sprandbool</string>

then we're good to merge

@KristofferC
Copy link
Sponsor Member Author

Doesn't it still need to be exported? Or does the deprecated macro export?

@tkelman
Copy link
Contributor

tkelman commented Apr 30, 2016

the deprecate macro should be handling the exporting, I believe

@nalimilan
Copy link
Member

the deprecate macro should be handling the exporting, I believe

That's right (as documented in the comment at the top of deprecated.jl).

@KristofferC
Copy link
Sponsor Member Author

I accidentally amended the latest changes to the "remove documentation" commit. However, if things are fine now then it will be squashed by the one who merges anyway so it shouldn't matter much.

@KristofferC
Copy link
Sponsor Member Author

Anyway this is ready from my pov.

@tkelman tkelman merged commit bbc8e60 into JuliaLang:master Apr 30, 2016
@KristofferC KristofferC deleted the kc/rand_type branch April 30, 2016 12:54
@ViralBShah ViralBShah added the domain:arrays:sparse Sparse arrays label Apr 30, 2016
@@ -152,7 +152,7 @@ end
let r1 = MersenneTwister(), r2 = MersenneTwister()
@test sprand(r1, 100, .9) == sprand(r2, 100, .9)
@test sprandn(r1, 100, .9) == sprandn(r2, 100, .9)
@test sprandbool(r1, 100, .9) == sprandbool(r2, 100, .9)
@test sprand(r1, Bool, 100, .9, ) == sprand(r2, Bool, 100, .9)
Copy link
Contributor

Choose a reason for hiding this comment

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

any recollection why there's an extra comma here?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Accident probably.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate sprandbool
4 participants