Skip to content

Use cholespy for CHOLMOD instead of scikit-sparse - #127

Merged
m-reuter merged 3 commits into
Deep-MI:mainfrom
m-reuter:cholespy
Oct 11, 2026
Merged

m-reuter merged 3 commits into
Deep-MI:mainfrom
m-reuter:cholespy

Conversation

@m-reuter

@m-reuter m-reuter commented Oct 10, 2026 •

Copy link
Copy Markdown
Member

Closes #119.

use_cholmod=True needed scikit-sparse, which is hard to install with pip, and the chol extra (scikit-sparse>=0.4.16) resolved to scikit-sparse 0.5.0, whose cholesky() returns a tuple, so Solver.eigs failed 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

  • New private module lapy/_cholmod.py with one shared CholeskyFactor used at all five call sites: Solver.eigs, Solver.poisson, heat.diffusion, diffgeo.tria_mean_curvature_flow and the solver behind conformal.spherical_conformal_map and spherical_tutte_map. It
    • passes the index arrays as int32 after checking that the matrix fits, and raises a clear error instead of letting the indices wrap;
    • turns right hand sides into C-contiguous float64 arrays, including sparse matrices and np.matrix, which Solver.poisson produces with boundary conditions;
    • factorizes and solves in double precision, which is what CHOLMOD does on the CPU, and returns float32 solutions for a float32 solver, so Solver(..., dtype=np.float32) keeps its output type;
    • passes the matrix to cholespy as CSR, which for a symmetric matrix is the same as CSC: for CSC input cholespy reallocates the caller's arrays in place, which corrupts the heap and crashed the Windows tests;
    • reports a matrix that is not positive definite as a ValueError that suggests use_cholmod=False.
  • splu stays the default. use_cholmod keeps its name and meaning, so there is no API change, and asking for it without cholespy installed is still an ImportError, without a fallback.
  • scikit-sparse is gone: the chol extra is now cholespy>=2.0, the first release with wheels for Python 3.11.
  • CI installs chol in 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.
  • The README and the use_cholmod docstrings describe the new backend.
  • The geodesics tests lose a branch that tried scikit-sparse on a singular matrix and then asserted that it was not installed.

Tests

New lapy/utils/tests/test_cholmod.py, skipped when cholespy is missing, checks that CHOLMOD and LU agree to 1e-8:

  • eigenvalues, with the factor used as a LinearOperator inside ARPACK;
  • Poisson solutions with Dirichlet and Neumann conditions, for one and several right hand sides;
  • heat diffusion from several seed sets and mean curvature flow;
  • a float32 solver, which returns float32 output within 1e-4 of the double precision result;
  • the not positive definite case, a closed surface without boundary conditions.

The existing conformal test comparing both backends now runs instead of being skipped. With solve replaced 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.

@codecov

codecov Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.76923% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.05%. Comparing base (89355e5) to head (0f49a94).

Files with missing lines Patch % Lines
lapy/_cholmod.py 70.58% 5 Missing and 5 partials ⚠️
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     
Flag Coverage Δ
unittests 67.05% <80.76%> (+1.93%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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.

Comment thread lapy/_cholmod.py Outdated
Comment thread lapy/utils/tests/test_cholmod.py Outdated
@m-reuter
m-reuter merged commit 0bd39cd into Deep-MI:main Oct 11, 2026
15 checks passed
@m-reuter
m-reuter deleted the cholespy branch October 11, 2026 07:14
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.

Support CHOLMOD via cholespy instead of scikit-sparse

2 participants