Build with uv + vendored choldate; verified pure-NumPy replacement - #1
Merged
Conversation
…cholupdate function, as it was causing issues with installation. Additionally, I fixed a bug in the input validation for the subset selection functions.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace pyproject.toml with a uv-compatible spec: add requires-python >=3.10, drop Cython from dependencies and build-system.requires (it was only needed for choldate), and pin the build backend to setuptools>=64. Pin the interpreter for uv via .python-version (3.12) and commit the generated uv.lock. `uv sync` resolves and installs cleanly (including mosek) and `uv run python -c "from pycss.CSS import CSS; from pycss.chol import cholupdate"` succeeds. Baseline: `uv run pytest tests/ -q` -> 22 passed, 12 failed. Pre-existing baseline failures (not fixed here), all `AttributeError: module 'numpy' has no attribute 'in1d'` (numpy 2.x removed np.in1d): - tests/test_CSS.py::test_CSS - tests/test_CSS.py::test_from_data_vs_from_cov - tests/test_CSS.py::test_rsq_cutoffs - tests/test_subset_selection.py::test_greedy_css - tests/test_subset_selection.py::test_greedy_css_exclude_last - tests/test_subset_selection.py::test_swapping_css - tests/test_subset_selection.py::test_swapping_order - tests/test_subset_selection.py::test_swapping_and_greedy_agree - tests/test_subset_selection.py::test_ordozgoiti_example - tests/test_subset_selection.py::test_swapping_beats_greedy - tests/test_subset_selection.py::test_greedy_factor_subset_selection - tests/test_subset_selection.py::test_swapping_subset_factor_selection Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
np.in1d was removed in NumPy 2.0; np.isin is the drop-in replacement. Restores the 12 baseline test failures to passing (34/34). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Vendors modusdatascience/choldate (BSD-3-Clause) with the build modernization from upstream PR #8 so the original Cython implementation builds under uv. Dev-only dependency: pycss runtime does not require it. Replaces the git submodule approach from e8c0faa that never built. The upstream _choldate.pyx also required its companion _choldate.pxd (defines the FLOAT_t typedef and cimports numpy) to compile at all; both are vendored verbatim. No contingency from the build-failure ladder was needed once the .pxd was present — uv sync compiled clean on cython 3 with the unmodified `from libc.math cimport abs as cabs` line. Added vendor/choldate/.gitignore to keep generated build artifacts (build/, *.egg-info/, _choldate.c, *.so) out of version control. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Match choldate semantics (in-place on R and x, returns None) and use np.hypot for numerical stability instead of naive sqrt of squares. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Move shared test helpers to tests/helpers.py so the pure-NumPy chol.py tests run even when the dev-only choldate is absent (previously the module-level importorskip skipped all of test_chol.py transitively). - Document choldowndate's NaN behavior on PD-violating downdates (differs from Cython choldate, which returns a wrong finite factor). - Add license metadata to the vendored choldate pyproject. - Remove the empty .gitmodules left from the submodule removal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR modernizes the project’s build workflow around uv, vendors a build-fixed copy of the original Cython choldate package as a dev-only dependency, and switches pycss runtime behavior to a pure-NumPy Cholesky rank-1 update/downdate implementation with parity tests and reference benchmarks.
Changes:
- Add
vendor/choldate/as a uv dev dependency and introduce tests verifying the vendored extension against re-factorization. - Introduce
pycss/chol.py(pure NumPycholupdate/choldowndate) and updatepycss/subset_selection.pyto use it; replace removednp.in1dwithnp.isin. - Add benchmarks and update documentation to reflect the new build/test workflow and choldate replacement strategy.
Reviewed changes
Copilot reviewed 15 out of 32 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| vendor/choldate/setup.py | Setuptools/Cython extension build entrypoint for vendored choldate. |
| vendor/choldate/pyproject.toml | PEP 518 build-system metadata for vendored choldate (dev-only). |
| vendor/choldate/LICENSE | Vendored BSD-style license text. |
| vendor/choldate/choldate/_choldate.pyx | Vendored Cython implementation of cholupdate/choldowndate. |
| vendor/choldate/choldate/_choldate.pxd | Cython declarations for the extension API/types. |
| vendor/choldate/choldate/init.py | Re-exports cholupdate/choldowndate from the extension. |
| vendor/choldate/.gitignore | Ignores build artifacts inside vendored package directory. |
| pyproject.toml | Defines uv-managed project metadata and wires vendored choldate into dev deps. |
| .python-version | Pins Python version for uv tooling. |
| pycss/chol.py | Adds pure-NumPy drop-in implementations for cholupdate/choldowndate. |
| pycss/subset_selection.py | Switches to internal cholupdate + replaces np.in1d with np.isin and fixes exclude-dup checks. |
| tests/init.py | Makes tests importable for helper sharing. |
| tests/helpers.py | Adds shared test helpers for SPD matrix generation and upper-tri factor extraction. |
| tests/test_chol.py | Validates pure-NumPy cholupdate/choldowndate vs refactorization + parity vs vendored choldate when present. |
| tests/test_choldate_vendored.py | Validates vendored choldate against direct re-factorization and in-place semantics. |
| benchmarks/bench_chol.py | Reference benchmark comparing NumPy vs Cython per-call and end-to-end swapping_css. |
| benchmarks/RESULTS.md | Captures benchmark results for reference. |
| README.md | Updates installation/test instructions to the uv workflow and documents the choldate replacement approach. |
| docs/superpowers/plans/2026-07-17-choldate-build-and-numpy-replacement.md | Adds an implementation plan document describing the migration and verification steps. |
| AGENTS.md | Adds a directory map reference document. |
| pycss.egg-info/SOURCES.txt | Updates tracked packaging artifact (sdist file list). |
| pycss.egg-info/requires.txt | Updates tracked packaging artifact (install requirements list). |
| pycss.egg-info/PKG-INFO | Updates tracked packaging artifact (distribution metadata). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+33
to
+36
| for k in range(p): | ||
| r = np.sqrt((R[k, k] - x[k]) * (R[k, k] + x[k])) | ||
| c = r / R[k, k] | ||
| s = x[k] / R[k, k] |
Owner
Author
There was a problem hiding this comment.
The factored form is correctly rounded exactly where rypot loses ~40 bits (near-vanishing pivots); the over/underflow regimes it protects against are unreachable for covariance-scale inputs; and the NaN behavior is documented, intentional, and safer than the Cython version's silent wrong answer.
Comment on lines
+10
to
+12
| scs | ||
| Cython | ||
| pingouin |
Comment on lines
+16
to
+18
| Requires-Dist: scs | ||
| Requires-Dist: Cython | ||
| Requires-Dist: pingouin |
Comment on lines
16
to
18
| tests/test_CSS.py | ||
| tests/test_PCSS.py | ||
| tests/test_subset_selection.py | ||
| tests/test_utils.py No newline at end of file |
pycss.egg-info/, __pycache__/, *.pyc, and .DS_Store were tracked from the original repo import and were never meant to be committed. Also addresses two of the Copilot review findings on PR #1 (stale Cython runtime dep and stale SOURCES.txt) by removing the generated egg-info from tracking entirely rather than trying to keep it in sync. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Missed staging this in the previous commit alongside untracking the same files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
Makes the fork a clean uv-managed project, vendors the original Cython
choldate(build-fixed) as a dev-only dependency, and replaces it at runtime with a verified pure-NumPy implementation.pyproject.toml(drops the unused Cython build dep, addsrequires-python >=3.10),.python-version(3.12), committeduv.lock. Workflow isuv sync+uv run pytest tests/.np.in1d(removed in NumPy 2.0) →np.isinat 4 sites inpycss/subset_selection.py; restores 12 previously failing tests.vendor/choldate/with the PEP 518 build fixes from modusdatascience/choldate#8 (plus the upstream companion_choldate.pxdthe build actually requires). Compiles clean under Cython 3 viauv sync; strictly dev-only — pycss runtime stays pure Python.pycss/chol.pynow providescholupdateandcholdowndateas exact drop-ins for the Cython originals (in-place onRandx, returnNone, numerically stablenp.hypot). Parity with vendored choldate verified to rtol=1e-12 (tests/test_chol.py); vendored build itself verified against direct re-factorization (tests/test_choldate_vendored.py). The NumPy tests still run (not skip) if choldate is absent.benchmarks/: Cython is ~38x faster per call at p=100, ~6x at p=1000; end-to-endswapping_cssdifference is small. Reference only — optimization is not a goal.choldategitlink, empty.gitmodules) removed.Test plan
uv syncfrom clean checkout compiles the vendored Cython extensionuv run pytest tests/ -q— 66 passed🤖 Generated with Claude Code