Repository navigation
Use cholespy for CHOLMOD instead of scikit-sparse - #127
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #127 +/- ##
==========================================
+ Coverage 65.11% 67.05% +1.93%
==========================================
Files 18 19 +1
Lines 3555 3573 +18
Branches 455 460 +5
==========================================
+ Hits 2315 2396 +81
+ Misses 1059 994 -65
- Partials 181 183 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
Backend behavior is comprehensively exercised across supported platforms, with only minor documentation precision corrections noted.
2 open findings
What changed in this PR
Replaces scikit-sparse with cholespy for the optional CHOLMOD backend.
Changes:
- Adds a shared CHOLMOD factorization wrapper.
- Migrates all solver call sites and documentation.
- Expands backend tests and CI coverage.
| File | Description |
|---|---|
README.md |
Documents cholespy installation. |
pyproject.toml |
Replaces the optional dependency. |
lapy/_cholmod.py |
Adds the shared factorization wrapper. |
lapy/solver.py |
Migrates eigenvalue and Poisson solves. |
lapy/heat.py |
Migrates heat diffusion. |
lapy/diffgeo.py |
Migrates curvature flow. |
lapy/conformal.py |
Migrates conformal solves. |
lapy/shapedna.py |
Updates backend documentation. |
lapy/utils/_imports.py |
Removes obsolete package mapping. |
lapy/utils/tests/test_cholmod.py |
Adds CHOLMOD/LU comparison tests. |
lapy/utils/tests/test_conformal.py |
Enables cholespy comparison testing. |
lapy/utils/tests/test_TriaMesh_Geodesics.py |
Removes obsolete singular Cholesky branch. |
lapy/utils/tests/test_TetMesh_Geodesics.py |
Removes obsolete singular Cholesky branch. |
.github/workflows/pytest.yml |
Tests cholespy across CI environments. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Closes #119.
use_cholmod=Trueneeded scikit-sparse, which is hard to install with pip, and thecholextra (scikit-sparse>=0.4.16) resolved to scikit-sparse 0.5.0, whosecholesky()returns a tuple, soSolver.eigsfailed with'tuple' object is not callable. This PR switches the CHOLMOD backend to cholespy, a small pip-installable package around the same CHOLMOD library, as proposed and benchmarked by @izachp in #119, following the design agreed there.Changes
lapy/_cholmod.pywith one sharedCholeskyFactorused at all five call sites:Solver.eigs,Solver.poisson,heat.diffusion,diffgeo.tria_mean_curvature_flowand the solver behindconformal.spherical_conformal_mapandspherical_tutte_map. Itint32after checking that the matrix fits, and raises a clear error instead of letting the indices wrap;float64arrays, including sparse matrices andnp.matrix, whichSolver.poissonproduces with boundary conditions;float32solutions for afloat32solver, soSolver(..., dtype=np.float32)keeps its output type;ValueErrorthat suggestsuse_cholmod=False.splustays the default.use_cholmodkeeps its name and meaning, so there is no API change, and asking for it without cholespy installed is still anImportError, without a fallback.cholextra is nowcholespy>=2.0, the first release with wheels for Python 3.11.cholin the test jobs, so the CHOLMOD path runs on every OS and Python version, and the minimum-versions job installs.[chol]with the lowest allowed versions, which tests cholespy 2.0.use_cholmoddocstrings describe the new backend.Tests
New
lapy/utils/tests/test_cholmod.py, skipped when cholespy is missing, checks that CHOLMOD and LU agree to 1e-8:LinearOperatorinside ARPACK;float32solver, which returnsfloat32output within 1e-4 of the double precision result;The existing conformal test comparing both backends now runs instead of being skipped. With
solvereplaced by the identity, every comparison test fails, so they exercise the backend. The suite passes with the current dependencies, with the minimum versions plus cholespy 2.0.0, and with the minimum versions without cholespy, where the CHOLMOD tests are skipped. The docs build with-W.Thanks to @izachp for the proposal and the benchmarks in #119.