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 LinearAlgebra.jl independent of SparseArrays.jl #43127

Merged
merged 2 commits into from
Dec 21, 2021
Merged

Conversation

dkarrasch
Copy link
Member

@dkarrasch dkarrasch commented Nov 18, 2021

First of all, this is mostly just code shuffling. Functional changes should be limited to:

  • concatenation of special "sparse" matrices does no longer result in sparse matrices, unless at least one argument is of Sparse* type;
  • 3-arg similar of specialized "sparse" matrices returns dense zeros. Besides additional memory consumption, this may be problematic for the matrix-of-matrices case;
  • in SVD we concatenate dense matrices and Diagonals in the D1 and D2 fields; these now return a dense matrix instead of a SparseMatrixCSC. OTOH, these fields are computed only upon getproperty calls, so shouldn't result in larger memory consumption by default.

@KristofferC
Copy link
Member

I wonder if it would be good to provide sparse_xcat functions that does what the old xcat with "sparse" special arrays (like Diagonal) did. I'm thinking in cases people really need to get a sparse array out of the catting with these special matrix types, they can at least swap to using the new sparse cat function.

@dkarrasch
Copy link
Member Author

The second commit now has sparse_*cat methods and a few tests. Maybe we should add that in a different PR, or at least keep the commits separate?

@dkarrasch dkarrasch marked this pull request as ready for review November 19, 2021 17:38
@ViralBShah
Copy link
Member

The separate commits sound like a good idea. Will the separate commits be cleanly and separately reversible should we need to?

@KristofferC
Copy link
Member

@nanosoldier runtests(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@dkarrasch dkarrasch force-pushed the dk/freelinalg branch 2 times, most recently from 5886095 to db3658e Compare November 24, 2021 16:29
@dkarrasch
Copy link
Member Author

Pkg eval didn't give any related errors, AFAICT. I rearranged the commits to reflect the two-step procedure.

@ViralBShah
Copy link
Member

ViralBShah commented Dec 2, 2021

I rebased it to master to see if it helps us pass linux32 and aarch64.

I feel like it is worth trying this out while we are still some ways away from code freeze. Would it be better to squash this all into one commit, should we need to revert it?

@dkarrasch
Copy link
Member Author

Sorry, I must have missed your message. So all tests pass, let's run pgk eval again. Otherwise, I agree it would be good to give this some time in the nightly builds to see whether it triggers serious errors or concerns. And yes, perhaps squashing is fine, because the sparse_cat functions make little sense without the other. I could keep the commits separated locally to remember which code changes are related to which aspect.

@nanosoldier runtests(ALL, vs = ":master")

@ViralBShah
Copy link
Member

@KristofferC how do you feel about this?

@KristofferC
Copy link
Member

If PkgEval looks good then I would say we should do this. It is one of the necessary steps to move out SparseArrays at least.

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@dkarrasch
Copy link
Member Author

dkarrasch commented Dec 9, 2021

@haampie IncompleteLU.jl failed in the PkgEval run with this error message:

Read SparseMatrixCSC row by row: Test Failed at /home/pkgeval/.julia/packages/IncompleteLU/UBpTh/test/linked_list.jl:36
  Expression: nzval(reader, column) == A[row, column]
   Evaluated: 0.36085659569841 == 0.0

see https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_hash/7d3df0d_vs_8197c41/IncompleteLU.1.8.0-DEV-8d8c8fe49ce.log. Would you mind taking a look to see if this is related to this PR? I wouldn't know how values of matrix entries change (edit: due to this PR).

@mcognetta
Copy link
Contributor

similar for structured matrices returning a dense matrix will be a surprise to users I think. Though having it return a sparse matrix like before was also a surprise..

@KristofferC
Copy link
Member

@nanosoldier runtests(["BayesianQuadrature", "BioServices", "ExpressCommands", "GaussianMixtureAlignment", "GridapDistributed", "IncompleteLU", "KrigingEstimators", "QuantumPropagators", "SLEEFPirates", "SimpleBufferStream", "SymbolicUtils", "VideoIO"], vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - no new issues were detected. A full report can be found here.

@dkarrasch
Copy link
Member Author

That nanosoldier run was helpful. @KristofferC what do you think, squash or not?

@KristofferC
Copy link
Member

KristofferC commented Dec 14, 2021

Seems like two commits are pretty good here, one for the change to xcat and one for the new sparse_xcat?

@dkarrasch
Copy link
Member Author

Ok, how do I do that (I'm used to the "squash and merge" option)? Just "create a merge commit" instead?

@oscardssmith
Copy link
Member

first squash to 2 commits locally and rebase on master. Then force push, and merge the merge commit.

@ViralBShah
Copy link
Member

@KristofferC Perhaps now we should be in a good place to pull out SparesArrays.jl into a separate repo. Is that something you can revive?

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

Successfully merging this pull request may close these issues.

6 participants