-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Make LinearAlgebra.jl independent of SparseArrays.jl #43127
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
Conversation
19e361e to
e840015
Compare
|
I wonder if it would be good to provide |
e840015 to
b94b83b
Compare
|
The second commit now has |
|
The separate commits sound like a good idea. Will the separate commits be cleanly and separately reversible should we need to? |
|
@nanosoldier |
|
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. |
5886095 to
db3658e
Compare
|
Pkg eval didn't give any related errors, AFAICT. I rearranged the commits to reflect the two-step procedure. |
|
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? |
db3658e to
7d3df0d
Compare
|
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 |
|
@KristofferC how do you feel about this? |
|
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. |
|
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. |
|
@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.0see 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). |
|
|
|
@nanosoldier |
|
Your package evaluation job has completed - no new issues were detected. A full report can be found here. |
|
That nanosoldier run was helpful. @KristofferC what do you think, squash or not? |
|
Seems like two commits are pretty good here, one for the change to |
|
Ok, how do I do that (I'm used to the "squash and merge" option)? Just "create a merge commit" instead? |
|
first squash to 2 commits locally and rebase on |
ec160b2 to
068c55d
Compare
|
@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? |
First of all, this is mostly just code shuffling. Functional changes should be limited to:
Sparse*type;similarof specialized "sparse" matrices returns densezeros. Besides additional memory consumption, this may be problematic for the matrix-of-matrices case;Diagonals in theD1andD2fields; these now return a dense matrix instead of a SparseMatrixCSC. OTOH, these fields are computed only upongetpropertycalls, so shouldn't result in larger memory consumption by default.