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

[WIP] generic sparse indices #40103

Closed
wants to merge 12 commits into from
Closed

Conversation

matbesancon
Copy link
Contributor

This tackles https://github.com/JuliaLang/julia/issues/31330
Linked issue: https://github.com/JuliaLang/julia/issues/25118

Will solve in a cleaner way: https://github.com/JuliaLang/julia/issues/39887

Design goal:
a generic index iterator for structural non-zeros of abstract arrays. This will allow efficient generic algorithms exploiting sparsity.

The function is defined in Base, so it can be implemented by the types in both LinearAlgebra and SparseArrays, I found it awkward to have it defined in LinearAlgebra itself

ping @dkarrasch

@matbesancon matbesancon marked this pull request as draft March 18, 2021 22:21
Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

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

I'm curious how you're going to implement the iterator for *Triangulars and *Diagonals. I was playing around with a little predicate-based draft. It turned out to improve on the current state, but wasn't nearly as fast as your latest Diagonal contribution.

stdlib/LinearAlgebra/src/diagonal.jl Outdated Show resolved Hide resolved
@matbesancon
Copy link
Contributor Author

I'm curious how you're going to implement the iterator for *Triangulars and *Diagonals.

I think some indices might require allocating? If you have some funky array where there non-zero pattern is well-defined but not trivially implementable with ranges or CartesianIndices.

The compromise would be:
--> non-allocating indices struct: more zeros
--> possibly arbitrary indices container: true structural non-zeros

It turned out to improve on the current state, but wasn't nearly as fast as your latest Diagonal contribution

I don't think we will do as fast as specialized methods for end-functions with specific types (like dot on Diagonal). However, this can still be a better base than having to rely on eachindex

@ViralBShah ViralBShah added the sparse Sparse arrays label Mar 20, 2021
@matbesancon
Copy link
Contributor Author

matbesancon commented Mar 21, 2021

@dkarrasch looking at SparseArrays.findnz, I think it could be ok for eachstoredindex to allocate a vector of non-zeros in the case where a smart type is not possible IMO

@matbesancon
Copy link
Contributor Author

Not sure why SparseArrays does not depend on Printf, it seems like it should?

@dkarrasch
Copy link
Member

@dkarrasch looking at SparseArrays.findnz, I think it could be ok for eachstoredindex to allocate a vector of non-zeros in the case where a smart type is not possible IMO

I guess it depends on where you want to use it. I think we rarely, if ever, call findnz internally, but rather work with the fields directly and thereby avoid allocations. In any case, for sparse matrices, I think you want to consider introducing a

getindex(A::AbstractSparseMatrixCSC, I::CartesianIndex{2}) = getindex(A, I[1], I[2])

method.

@matbesancon
Copy link
Contributor Author

matbesancon commented Mar 22, 2021

I would be ok with adding the method, but it already has the corresponding behaviour implemented with the current fallback:

julia> using SparseArrays

julia> m = spzeros(200,100);

julia> m[CartesianIndex(1,1)]
0.0

why do you have it in mind?

@dkarrasch
Copy link
Member

why do you have it in mind?

Because I saw that we have such a method specifically for tuples of integers, so it seemed we don't rely on fallbacks there either. But otherwise no other reason.

matbesancon and others added 3 commits March 24, 2021 15:40
"""
eachstoredindex(A)

Returns an iterable over the indices of `A` where the values are structurally non-zero.
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to define this as being tied to structural nonzeros? Or simply stored values? We could simultaneously introduce a unstoredvalue to give room for other sorts of structures.

In any case, I like this and think it's very needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. What would be expected for a FillArrays.Ones? In the case of stored values, it would return an empty iterator? I am not sure we can make this useful in general, except for constructors?

@DilumAluthge
Copy link
Member

@matbesancon It looks like this PR touches seven files. Two of those files (base/abstractarray.jl and NEWS.md) are in Base, one of those files (stdlib/LinearAlgebra/src/diagonal.jl) is in the LinearAlgebra stdlib, and the remaining four files are in the SparseArrays stdlib.

We have moved the SparseArrays stdlib to an external repository. Therefore, for the portion of this PR that modifies the SparseArrays stdlib, can you please:

  1. Remove the SparseArrays changes from this PR.
  2. Open a new PR against https://github.com/JuliaLang/SparseArrays.jl with the SparseArrays changes.

Thank you!

@DilumAluthge DilumAluthge added arrays [a, r, r, a, y, s] linear algebra Linear algebra and removed sparse Sparse arrays labels Jan 14, 2022
@ViralBShah ViralBShah closed this Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants