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

Merge SuiteSparse into SparseArrays to prepare for excision from base #95

Merged
merged 6 commits into from
May 30, 2022

Conversation

rayegun
Copy link
Member

@rayegun rayegun commented May 12, 2022

This is a very simple copy-paste attempt to solve JuliaLang/julia#44247 while I'm away from the office/home (I'm also not 100% sure it's all working; I don't have a good dev set up on this device).

What exactly should go in SuiteSparse to avoid breaking changes is an open question. A re-export doesn't really make sense since SuiteSparse didn't do any exporting. The only package I know of that might mess with SuiteSparse directly is LinearSolve.jl which I can update.

We could do the following with the SuiteSparse.jl stdlib

  1. Deprecate SuiteSparse.jl entirely (this PR is the SparseArrays.jl side of doing that).
  2. Keep the generated wrappers from lib and gen in SuiteSparse.jl.
  3. Redevelop SuiteSparse to just be lower level wrappers (it's not extremely onerous, but involves separating functionality in a somewhat ugly way between SuiteSparse.jl and SparseArrays.jl, and making pretty substantial changes to the current data structures)

Note: this is being done because currently the dependency chain is SparseArrays->SuiteSparse. However, by removing SparseArrays and SuiteSparse from the sysimage we lose the ability to do / on SparseMatrixCSC without using SuiteSparse. The simplest solution is to combine SuiteSparse and SparseArrays.

E: Still need to copy tests

@codecov-commenter
Copy link

codecov-commenter commented May 12, 2022

Codecov Report

Merging #95 (09c7f6e) into main (1197f17) will decrease coverage by 45.02%.
The diff coverage is 89.27%.

@@             Coverage Diff             @@
##             main      #95       +/-   ##
===========================================
- Coverage   89.06%   44.04%   -45.03%     
===========================================
  Files           7       11        +4     
  Lines        5679     7039     +1360     
===========================================
- Hits         5058     3100     -1958     
- Misses        621     3939     +3318     
Impacted Files Coverage Δ
src/solvers/LibSuiteSparse.jl 0.00% <0.00%> (ø)
src/solvers/umfpack.jl 86.43% <86.43%> (ø)
src/solvers/spqr.jl 89.33% <89.33%> (ø)
src/solvers/cholmod.jl 90.67% <90.67%> (ø)
src/SparseArrays.jl 80.00% <100.00%> (-20.00%) ⬇️
src/sparsevector.jl 15.58% <0.00%> (-78.59%) ⬇️
src/higherorderfns.jl 31.20% <0.00%> (-65.23%) ⬇️
src/sparsematrix.jl 37.77% <0.00%> (-56.99%) ⬇️
src/sparseconvert.jl 33.33% <0.00%> (-52.23%) ⬇️
... and 2 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@rayegun
Copy link
Member Author

rayegun commented May 12, 2022

What's the best way to test this? Remove SuiteSparse from the Julia build entirely and make?

@rayegun
Copy link
Member Author

rayegun commented May 14, 2022

I believe this should now work; I've tested against a local 1.9 build. Although I will do more rigorous tests once I'm home Wednesday of next week. @ViralBShah

@ViralBShah
Copy link
Member

@KristofferC

@rayegun rayegun requested a review from KristofferC May 15, 2022 18:19
@KristofferC
Copy link
Member

I think it would be best to completely empty Suitesparse and just have it load SparseArrays.SuiteSparse so that things are non breaking. IIUC this is what has been done here.

@rayegun
Copy link
Member Author

rayegun commented May 19, 2022

I did flatten things somewhat, so rather than SparseArrays.SuiteSparse.CHOLMOD it's just SparseArrays.CHOLMOD. There's only two functions in SparseArrays.SuiteSparse that aren't in an inner module: increment[!] and decrement[!]. These are both useful for SparseArrays in general.

@ViralBShah
Copy link
Member

There's a docs error - which should be easy to fix. After that let's merge and empty out SuiteSparse.jl as discussed above (so that it just loads SparseArrays.SuiteSparse).

@ViralBShah ViralBShah changed the title Transfer SuiteSparse Merge SuiteSparse into SparseArrays to prepare for excision from base May 29, 2022
@ViralBShah
Copy link
Member

@Wimmerer We need to empty out the SuiteSparse package too at the same time, so that we merge and bump both simultaneously.

@rayegun
Copy link
Member Author

rayegun commented May 30, 2022

There's an equivalent PR for SS

@ViralBShah
Copy link
Member

ViralBShah commented May 30, 2022

How do you want to handle JuliaSparse/SuiteSparse.jl#67? The diff seems simple enough that we can pull it in here after merging this.

This has been given enough thought. Let's merge in both repos.

@ViralBShah
Copy link
Member

ViralBShah commented May 30, 2022

Note that upon excision from base, we probably want to use v1.0.0 for SparseArrays (when registering in General) in order to signal seriousness about API stability.

@rayegun
Copy link
Member Author

rayegun commented May 30, 2022

We'll probably just have @SobhanMP remake the PR here right? It's simple enough to move.

Before we merge this I do want to check with you to be sure that flattening the module hierarchy was ok. We went from SuiteSparse.{CHOLMOD | UMFPACK | SPQR} -> SparseArrays.{CHOLMOD | UMFPACK | SPQR} rather than SparseArrays.SuiteSparse.{CHOLMOD | UMFPACK | SPQR}. It felt unnecessarily deep to go with the last option.

@ViralBShah
Copy link
Member

We need to use the module hierarchy that will not break existing Julia packages from Julia 1.0 onwards.

@rayegun
Copy link
Member Author

rayegun commented May 30, 2022

SuiteSparse will still be exactly the same, this is about entirely new hierarchy for SparseArrays.

See: https://github.com/JuliaSparse/SuiteSparse.jl/blob/b213ec867c4815c1ee13d9377ec4835dc435390c/src/SuiteSparse.jl

@ViralBShah
Copy link
Member

It seems fine to me (and we have enough time for 1.9 to figure out what to fix in case something needs to be done differently).

@rayegun
Copy link
Member Author

rayegun commented May 30, 2022

Okay merging

@rayegun rayegun merged commit a15fe4b into JuliaSparse:main May 30, 2022
@rayegun rayegun deleted the transfer_suitesparse branch May 30, 2022 20:48
rayegun added a commit to JuliaSparse/SuiteSparse.jl that referenced this pull request May 30, 2022
@ViralBShah
Copy link
Member

The SuiteSparse docs are not yet showing up in the SparseArray docs.

@rayegun
Copy link
Member Author

rayegun commented Jun 2, 2022

I probably neglected to transfer them. I will do that now.

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.

4 participants