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

Rename issparse? #24645

Closed
garrison opened this issue Nov 18, 2017 · 5 comments
Closed

Rename issparse? #24645

garrison opened this issue Nov 18, 2017 · 5 comments
Labels
sparse Sparse arrays

Comments

@garrison
Copy link
Member

... aka Bikeshed and/or improve documentation for issparse. The docstring for issparse says

issparse(S)

Returns true if S is sparse, and false otherwise.

but this is ambiguous. What, precisely, does it mean for S (presumably an array) to be sparse? Here are two (very different) possible definitions:

  1. The array has a density of nonzero elements less than some parameter x (x could have some default value, but this would be rather arbitrary).
  2. The array is represented by a some "sparse" data type.

It is the second option that is implemented by issparse. In a way this makes sense, as the function does not take such a parameter x (representing the threshold density of nonzero elements). But at a minimum this should be more clearly explained in the documentation. (I'm also unable to explain why issparse(::Diagonal) and related types return false -- perhaps it's a bug, or perhaps I do not understand fully what this function is meant to do.)

On the other hand, the current behavior makes issparse different from many other functions I can think of, such as istril, istriu, issymmetric, ishermitian, and isreal. Each of these considers the value itself rather than the datatype; for instance, isreal(0im) == true.

Perhaps a better name for this function, then, would be issparsetype? I can think of downsides to this name (you do not pass a type to it). Another idea would be issparsestorage, but I don't know if I like this either.

So I ask:

  1. Do people agree that issparse is a poor name? If it is, can we come up with anything better?
  2. What should issparse(::Diagonal), issparse(::Tridiagonal), etc. return?
  3. Is this function even useful? I just searched discourse, and it's never been mentioned there. In Base,
    it is used only in the concatenation functions in sparsevector.jl. I have not grepped the package ecosystem, but I wonder how frequently it is used there.
@garrison garrison added the sparse Sparse arrays label Nov 18, 2017
@andreasnoack
Copy link
Member

The methods are very old and most likely inherited from Matlab. In Julia, I think it makes more sense to check if the type is a SparseMatrixCSC or AbstractSparseMatrix but there is the complication that a this would be false for e.g. LowerTriangular{SparseMatrixCSC} for which issparse is true.

@oxinabox
Copy link
Contributor

oxinabox commented Nov 22, 2017

issparse should be a property of the type only.
Like a trait.

as @andreasnoack said it can't be just replaced with:

x isa AbstractSparseMatrix as it also depends on type parameters for e.g. LowerTriangular.

Its currently defined in

issparse(A::AbstractArray) = false
issparse(S::AbstractSparseArray) = true
issparse(S::Symmetric{<:Any,<:AbstractSparseMatrix}) = true
issparse(S::Hermitian{<:Any,<:AbstractSparseMatrix}) = true
issparse(S::LowerTriangular{<:Any,<:AbstractSparseMatrix}) = true
issparse(S::LinAlg.UnitLowerTriangular{<:Any,<:AbstractSparseMatrix}) = true
issparse(S::UpperTriangular{<:Any,<:AbstractSparseMatrix}) = true
issparse(S::LinAlg.UnitUpperTriangular{<:Any,<:AbstractSparseMatrix}) = true
indtype(S::AbstractSparseArray{<:Any,Ti}) where {Ti} = Ti

It would be marginally cleaner if it were:

issparse(x:T) where {T} = issparse(T)

issparse(::Type{AbstractArray}) = false
issparse(::Type{AbstractSparseArray}) = true
issparse(::Type{Symmetric{<:Any, T}}) where {T} = issparse(T)

But is it an useful trait is a good question.

Having it as a trait might be marginally cleaner in

function cat(catdims, Xin::_SparseConcatGroup...)

@garrison
Copy link
Member Author

Here is a list of uses of issparse in the julia package ecosystem, created using cloneallpackages and grep.

@ViralBShah
Copy link
Member

I like the isa solution offered above, and if people are generally onboard, it shouldn't be difficult to get this changed in all the packages.

Perhaps FemtoCleaner can automate it all.

@User-764Q
Copy link

HI,

It looks like this was merged with #34308 in Dec 18. Does that mean this issue can be closed?

Matt.

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

No branches or pull requests

5 participants