-
Notifications
You must be signed in to change notification settings - Fork 8
v2.0: remove almost all solver, preconditioner infrastructure in favor of LinearSolve #63
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
pjaap
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impressive! I found only a minor leftover in the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR performs a major v2.0 refactoring of ExtendableSparse.jl, removing the old solver and preconditioner infrastructure in favor of relying primarily on LinearSolve.jl. The key architectural change is replacing the old ExtendableSparseMatrixCSC struct with a modular GenericExtendableSparseMatrixCSC{SparseMatrixLNK} approach, making the package more flexible and easier to understand.
- Removes deprecated factorization and preconditioner infrastructure
- Restructures to use
GenericExtendableSparseMatrixCSCwith pluggable sparse matrix extensions - Adds comprehensive code quality checks (Aqua, ExplicitImports, UndocumentedNames)
Reviewed changes
Copilot reviewed 69 out of 70 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/ExtendableSparse.jl | Simplified main module, removed factorization exports, added @public macros |
| src/typealiases.jl | New file defining backward-compatible type aliases |
| src/abstractextendablesparsematrixcsc.jl | Enhanced documentation and API consistency with TYPEDSIGNATURES |
| src/sparsematrixlnk.jl | Fixed copy() implementation, updated documentation style |
| src/sparsematrixdilnkc.jl | Improved documentation, fixed field name consistency |
| src/sparsematrixdict.jl | Added comprehensive documentation, fixed I/J swap bug |
| src/preconbuilders.jl | Added BlockPreconditioner and JacobiPreconditioner implementations |
| src/sprand.jl | Updated to work with new abstract types |
| test/*.jl | Updated all tests to use new GenericExtendableSparseMatrixCSC API |
| docs/*.md | Restructured documentation to reflect new architecture |
| ext/*.jl | Removed deprecated Pardiso and AMG extensions |
Comments suppressed due to low confidence (4)
src/sparsematrixdilnkc.jl:489
- There's an inconsistency in variable naming: the field is
rowval(line 489) but was previously referenced asrowvalsin the old code. While this change is correct (matching the actual field name), ensure all references throughout the codebase userowvalconsistently.
src/sparsematrixdict.jl:161 - The swap of
IandJassignment appears to be a bug fix. Previously,I[i] = icscandJ[i] = rowval[j]meant columns were stored in I and rows in J, which is backwards for sparse matrix COO format where I typically holds row indices and J holds column indices. The fix correctly assignsI[i] = rowval[j](rows) andJ[i] = icsc(columns). However, verify that this doesn't break existing code that might have been compensating for the previous incorrect behavior.
src/sparsematrixcsc.jl:2 - The documentation string uses
$(TYPEDSIGNATURES)which generates detailed type signatures, but this appears on line 2 as a replacement for the previous simpler format. While this is more informative, ensure that the generated documentation doesn't become overly verbose or cluttered with internal implementation details that users don't need to see.
src/sparsematrixlnk.jl:411 - In the copy function, direct field assignment is used without calling
copy()on the arrays. This creates shallow copies where the new SparseMatrixLNK shares the same underlying arrays (colptr, rowval, nzval) with the original. This could lead to unexpected mutations if either the original or the copy is modified. Consider usingcopy(S.colptr),copy(S.rowval), andcopy(S.nzval)to ensure deep copies.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
(EDIT after #63e1238)
Besides removing old preconditioner structure, this PR overauls the general structure of the package.
It removes the old
ExtendableSparseMatrixCSCstruct in favor ofGenericExtendableSparseMatrixCSC{SparseMatrixLNK}, thus using the previously introduced modular infrastructure as default. This is more flexible, and, hopefully, also easier to understand.While formally breaking, I took care that the change is non-breaking for ExtendablFEMBase and ExtendableFEM. VoronoiFVM will need an upgrade to v3.0 in order to take this in.
Also, we have now Aqua, ExplicitImports and UndocumentedNames tests.