-
Notifications
You must be signed in to change notification settings - Fork 24
Replace Pinv default with BackslashSolver for better performance and Windows compatibility #121
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
base: master
Are you sure you want to change the base?
Conversation
…Windows compatibility This PR replaces the problematic `Pinv` default coarse solver with a new `BackslashSolver` that uses Julia's built-in `factorize()` and backslash operator, providing: ## Benefits - **Preserves sparsity**: No conversion to dense matrices - **Better performance**: Uses appropriate sparse factorizations (UMFPACK, CHOLMOD, etc.) - **Windows compatibility**: Eliminates LAPACK errors from `pinv` calls - **Simpler code**: Much cleaner than LinearSolve.jl wrappers - **Better accuracy**: Direct factorization vs pseudoinverse ## Changes - Add `BackslashSolver` implementation using `factorize(A)` and `A \ b` - Change default from `Pinv` to `BackslashSolver` in both Ruge-Stuben and Smoothed Aggregation - Add comprehensive tests for `BackslashSolver` - Deprecate `Pinv` with warning about inefficiency - Keep `Pinv` available for backward compatibility ## Fixes - Resolves SciML/Sundials.jl#496 (Windows LAPACK errors) - Dramatically improves memory usage and performance for coarse solves - Makes the default behavior much more intuitive for Julia users 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Why was this using |
pinv is the default on the coarse level, because for general matrices I am not against changing the defaults here, I just want to make sure that you are aware this might cause downstream issues. The failing test can be fixed by forcing these to use Pinv. |
but you can force a column-pivoted qr or svd factorization for that, without building the pinv. |
Summary
This PR replaces the problematic
Pinv
default coarse solver with a newBackslashSolver
that uses Julia's built-infactorize()
and backslash operator. This provides much better performance, preserves sparsity, and fixes Windows LAPACK compatibility issues.Problem
The current default coarse solver
Pinv
has serious issues:Matrix(A)
destroys sparsity, causing memory explosionSolution
The new
BackslashSolver
:factorize(A)
which automatically chooses appropriate sparse factorizations (UMFPACK, CHOLMOD, etc.)A \ b
Implementation
Changes
BackslashSolver
implementationPinv
toBackslashSolver
in Ruge-Stuben and Smoothed AggregationBackslashSolver
Pinv
with performance warning (kept for backward compatibility)Testing
BackslashSolver
handles multiple RHS correctlyPerformance Impact
The new default should provide:
Backward Compatibility
Pinv
is still available for users who explicitly request itFixes
This change makes AlgebraicMultigrid.jl's default behavior much more appropriate for sparse linear algebra and aligns with Julia's excellent built-in capabilities.
🤖 Generated with Claude Code