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

similar calls undefined method with sparse matrices and dimensions other than 1 or 2 #23905

Closed
marekdedic opened this issue Sep 27, 2017 · 8 comments
Labels
bug Indicates an unexpected problem or unintended behavior sparse Sparse arrays

Comments

@marekdedic
Copy link

When trying to call similar on a sparse array with empty tuple as last argument, an error is thrown:

julia> a = sparse([1, 2, 3])
3-element SparseVector{Int64,Int64} with 3 stored entries:
  [1]  =  1
  [2]  =  2
  [3]  =  3

julia> similar(a, ())
ERROR: MethodError: no method matching spzeros(::Type{Int64})
Closest candidates are:
  spzeros(::Type{T}, ::Integer) where T at sparse/sparsevector.jl:47
  spzeros(::Type{Tv}, ::Integer, ::Integer) where Tv at sparse/sparsematrix.jl:1375
  spzeros(::Type{Tv}, ::Type{Ti<:Integer}, ::Integer) where {Tv, Ti<:Integer} at sparse/sparsevector.jl:48
  ...
Stacktrace:
 [1] similar(::SparseVector{Int64,Int64}, ::Tuple{}) at ./abstractarray.jl:521

AFAIK the underlying issue is that sparse arrays are defined only for 1D or 2D, so the call to similar (which should create a 0D sparse array) is meaningless. This behaviour is definitely undocumented and would benefit from a clearer error message (provided n-dimensional sparse arrays are not implemented in the future)

julia> versioninfo()
Julia Version 0.6.0
Commit 903644385b (2017-06-19 13:05 UTC)
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: Intel(R) Core(TM) i5-6300U CPU @ 2.40GHz
  WORD_SIZE: 64
  BLAS: libopenblas (USE64BITINT DYNAMIC_ARCH NO_AFFINITY Haswell)
  LAPACK: libopenblas64_
  LIBM: libopenlibm
  LLVM: libLLVM-3.9.1 (ORCJIT, skylake)
@fredrikekre
Copy link
Member

What is the problem here? And what would you have expected instead of a MethodError?

@fredrikekre fredrikekre added the sparse Sparse arrays label Oct 1, 2017
@marekdedic
Copy link
Author

marekdedic commented Oct 1, 2017

The problem here is that from this error message it is not really clear what the problem is (i. e. trying to create a 0D sparse array which is not possible). Instead the error message is about function spzeros, which to me sounds very confusing when debugging in a larger codebase (been there, done that).

Also, such limitation would IMHO warrant a mention in the documentation.

The problem isn't the behaviour (throwing an error), but the clarity of the error message and the missing mention in the documentation

@nalimilan
Copy link
Member

Maybe similar should return a 0D dense Array in that case? That's a very reasonable structure for this situation, and there's not risk of running out of memory.

@marekdedic
Copy link
Author

That would make sense... This would need to be well thought through because you don't want this operation to accidentaly convert a sparse array to a dense one if that dense array is used anywhere else later on - that could cause some really cryptic errors for some edge cases (constructing 2D array from 0D arrays maybe?)

@Sacha0
Copy link
Member

Sacha0 commented Oct 16, 2017

similar(S::SparseMatrixCSC, [Tv,] shape) where shape is a Dims{0} or Dims{N>2} should work after #17507 (via the relevant AbstractArray fallback, which yields Arrays). On the other hand, similar(S::SparseMatrixCSC, Tv, Ti, shape) will fail with a simple, clear MethodError in the preceding cases, given index-type specification is specific to sparse arrays (and so the relevant methods have no AbstractArray fallbacks).

So the failure mode the OP describes should be confined to similar on SparseVectors after #17507. I have a pull request inbound which should make the behavior of similar for SparseVectors consistent with that for SparseMatrixCSCs.

The remaining question is whether the similar methods for sparse arrays that accept an index type should continue to MethodError thereafter or instead ignore the index type specification and fall back to the AbstractArray behavior (yielding Arrays). I lean towards providing the gentle fallback, ignoring the index type specification.

Thoughts? Best!

@Sacha0 Sacha0 added the bug Indicates an unexpected problem or unintended behavior label Oct 16, 2017
@Sacha0 Sacha0 modified the milestone: 0.6.x Oct 16, 2017
@Sacha0
Copy link
Member

Sacha0 commented Oct 16, 2017

The remaining question is whether the similar methods for sparse arrays that accept an index type should continue to MethodError thereafter or instead ignore the index type specification and fall back to the AbstractArray behavior (yielding Arrays). I lean towards providing the gentle fallback, ignoring the index type specification.

On the other hand, the similar variants with index type specification shouldn't appear in generic contexts / should only appear in sparse-specific contexts, so perhaps a MethodError is best.

@marekdedic
Copy link
Author

If it's used only in sparse operations, then I am against silent conversions. If it's not, I would opt for consistency.

@Sacha0
Copy link
Member

Sacha0 commented Oct 24, 2017

Fixed by #17507 and #24260 on master. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior sparse Sparse arrays
Projects
None yet
Development

No branches or pull requests

4 participants