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

Make issparse handle nested wrappers #34308

Merged
merged 3 commits into from
May 13, 2021
Merged

Conversation

oxinabox
Copy link
Contributor

@oxinabox oxinabox commented Jan 8, 2020

Follow up to: #34266

Handles all the existing cases, plus:

  • arbitary nesting of wrappers
  • SubArrays
  • Diagonal

It all constant folds out:

julia> z = adjoint(transpose((1 + im) .*sprandn(5,5, 0.5)));

julia> @code_llvm issparse(z)

;  @ REPL[18]:2 within `issparse'
define i8 @julia_issparse_17287(%jl_value_t addrspace(10)* nonnull align 8 dereferenceable(8)) {
top:
;  @ REPL[18]:4 within `issparse'
  ret i8 1
}

julia> q = adjoint(transpose((1 + im) .*randn(5,5)));

julia> @code_llvm issparse(q)

;  @ REPL[18]:2 within `issparse'
define i8 @julia_issparse_17303(%jl_value_t addrspace(10)* nonnull align 8 dereferenceable(8)) {
top:
;  @ REPL[18]:4 within `issparse'
  ret i8 0
}

@fredrikekre
Copy link
Member

Perhaps OT, but can we just delete issparse instead? It does not seem very useful, what do you do with it? Outside of tests it is only used in three places:

X = map(x -> SparseMatrixCSC(issparse(x) ? x : sparse(x)), Xin)
X = map(x -> SparseMatrixCSC(issparse(x) ? x : sparse(x)), Xin)
and
X = map(x -> SparseMatrixCSC(issparse(x) ? x : sparse(x)), Xin)

(See also #24645)

@oxinabox
Copy link
Contributor Author

oxinabox commented Jan 8, 2020

I agree with the sentiment.
a trait without a set of rules about what it promises is not good for much.

Apparently I also agreed back in #24645

But since it is a breaking change, we can't delete it.

So we might as well make it work well.

That may or may not include making it work for these cases.

Perhaps before doing this we should decide what it means.

It probably should mean something about the cost of various operations.

@oxinabox
Copy link
Contributor Author

oxinabox commented Jan 8, 2020

This errors because:

  • Diagonal(rand(4,4)) works fine and just cuts out the diagonal, ignoring nondiagonal elements.
  • Diagonal(LowerTriangular(rand(4,4))) errors because the matrix has zeros on the non-diagonal

Which seems really silly and inconsistent to me.

@ViralBShah
Copy link
Member

I thought we can't delete APIs that are in 1.0.

@oxinabox
Copy link
Contributor Author

Revisiting this. Since I apparently decided to open a second PR having forgotten i made this one.
#37644

What does it mean to be issparse?
I haven't seen a defination of an interface that must be furfilled.

Possibly all people want this for is to know if they have some algorthm that really won't run great on a sparse array, then they could issue a warning or run collect to make it dense.

@fredrikekre fredrikekre added the sparse Sparse arrays label Sep 18, 2020
@dkarrasch
Copy link
Member

The tests pass locally for me, so this should be good to go after a rebase.

@oxinabox
Copy link
Contributor Author

Done

@dkarrasch dkarrasch added the merge me PR is reviewed. Merge when all tests are passing label May 12, 2021
@vtjnash vtjnash merged commit 55c32b5 into JuliaLang:master May 13, 2021
@simeonschaub simeonschaub removed the merge me PR is reviewed. Merge when all tests are passing label May 21, 2021
shirodkara pushed a commit to shirodkara/julia that referenced this pull request Jun 9, 2021
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
@User-764Q User-764Q mentioned this pull request Sep 23, 2021
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

Successfully merging this pull request may close these issues.

6 participants