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

More generic SparseMatrixCSC and SparseVector #33918

Closed
wants to merge 3 commits into from

Conversation

Roger-luo
Copy link
Contributor

@Roger-luo Roger-luo commented Nov 22, 2019

This PR loose the original type bounds to AbstractVector since we are using a custom type in LuxurySparse (https://github.com/QuantumBFS/LuxurySparse.jl/blob/master/src/SSparseMatrixCSC.jl) to use SVector as storage for some not so small but very sparse matrices in quantum circuit, I think it is reasonable to just commit this to upstream.

This PR won't effect the current behaviour I think. And I think Printf is missing in current master?

@tkf
Copy link
Member

tkf commented Nov 22, 2019

I proposed exactly the same enhancement #30173 which was rejected. However, we now have AbstractSparseMatrixCSC as of #33039 which lets you do effectively the same thing. It will be included in Julia 1.4. Note that the public API hasn't been decided yet. See #33054 for a summary. For usage example, see tkf/SparseXX.jl#2

@ViralBShah
Copy link
Member

I would like to make progress on this general topic. I think the main issue on public API was that we can't break things - so we just need to ensure backward compatibility while getting a new public API out.

@Roger-luo
Copy link
Contributor Author

Roger-luo commented Nov 22, 2019

Oh I didn't notice your PR @tkf I'm just trying to commit our evil type piracies in LuxurySparse to upstream.

I think the main issue on public API was that we can't break things - so we just need to ensure backward compatibility while getting a new public API out.

Will this break anything? I think the behaviour will be exactly same as before except when someone implement methods to construct SparseMatrixCSC with other types?

@Roger-luo
Copy link
Contributor Author

I originally tried to commit it to StaticArrays but was suggested to commit this to upstream: JuliaArrays/StaticArrays.jl#660

@tkf
Copy link
Member

tkf commented Nov 22, 2019

Will this break anything?

I think this PR as-is is breaking in some cases. For example,

struct MyStruct{Tv,Ti}
    i_need_this_to_be_concrete::SparseMatrixCSC{Tv,Ti}
end

will have non-concrete field after this PR. It also changes printing of the type (which may be considered "minor change", though). We considered various approaches to this: #30173 (comment)

The blocker for AbstractSparseMatrixCSC #33054 is naming of the new functions. The discussion is happening in #25118.

@Roger-luo
Copy link
Contributor Author

I see. I'll close this then.

@Roger-luo Roger-luo closed this Nov 22, 2019
@Roger-luo Roger-luo deleted the roger/generic-sparse branch November 22, 2019 05:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants