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

Fix Issue #5985: sprandbool() fails and does not have unit tests #5986

Merged
merged 1 commit into from
Feb 28, 2014

Conversation

jperla
Copy link

@jperla jperla commented Feb 28, 2014

Fixes an issue where sprandbool fails

@@ -8,10 +8,10 @@ type SparseMatrixCSC{Tv,Ti<:Integer} <: AbstractSparseMatrix{Tv,Ti}
n::Int # Number of columns
colptr::Vector{Ti} # Column i is in colptr[i]:(colptr[i+1]-1)
rowval::Vector{Ti} # Row values of nonzeros
nzval::Vector{Tv} # Nonzero values
nzval::AbstractVector{Tv} # Nonzero values
Copy link
Member

Choose a reason for hiding this comment

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

The problem is that for efficiency reasons the fields should be concrete types not abstract. I think that it is better that nzval is a Vector{Bool} instead of a BitArray{1} for sprandbool.

@jperla
Copy link
Author

jperla commented Feb 28, 2014

But then that will be less efficient for boolean matrices.

Shouldn't the JIT take care of this?

@jperla
Copy link
Author

jperla commented Feb 28, 2014

@andreasnoackjensen what if I add a new Union type of BitArray{1} and Vector{T}?

@tkelman
Copy link
Contributor

tkelman commented Feb 28, 2014

A boolean sparse matrix doesn't really need a values array at all, the nonzeros are implicitly true. Can you express whatever sparse boolean matrix operation you're looking to do using something in Graphs.jl?

@JeffBezanson
Copy link
Member

The problem is that this means a given sparse matrix can change how its nzval vector is represented at different times, which is not desirable.

I think this patch is what we want:

diff --git a/base/sparse/sparsematrix.jl b/base/sparse/sparsematrix.jl
index b62ede3..0df0db7 100644
--- a/base/sparse/sparsematrix.jl
+++ b/base/sparse/sparsematrix.jl
@@ -389,7 +389,7 @@ function sprand(m::Integer, n::Integer, density::FloatingPoint, rng::Function, v
     if !iseltype(v,Bool)
         return sparse_IJ_sorted!(I, J, rng(length(uind)), m, n, +)  # it will never need to combine
     else
-        return sparse_IJ_sorted!(I, J, trues(length(uind)), m, n, +)
+        return sparse_IJ_sorted!(I, J, ones(Bool,length(uind)), m, n, +)
     end
 end

@timholy
Copy link
Member

timholy commented Feb 28, 2014

@jperla, the JIT can help only if the types can be determined at runtime. The issues here are discussed at length in the FAQ: http://docs.julialang.org/en/latest/manual/faq/#how-do-abstract-or-ambiguous-fields-in-types-interact-with-the-compiler and the next section, which deals directly with the issue here.

For a better design, see JuliaLang/LinearAlgebra.jl#84

@jperla
Copy link
Author

jperla commented Feb 28, 2014

@JeffBezanson okay i updated the code to use ones(), all tests pass. Thanks!

@jperla
Copy link
Author

jperla commented Feb 28, 2014

thanks @timholy

JeffBezanson added a commit that referenced this pull request Feb 28, 2014
Fix Issue #5985: sprandbool() fails and does not have unit tests
@JeffBezanson JeffBezanson merged commit 71e1276 into JuliaLang:master Feb 28, 2014
@ViralBShah ViralBShah added the randomness Random number generation and the Random stdlib label Nov 22, 2014
@ViralBShah
Copy link
Member

I think that the sprand API should be revisited and made similar to the other recent changes to rand. Also, we probably can do without sprandbool.

@tkelman tkelman mentioned this pull request Nov 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
randomness Random number generation and the Random stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants