Refactoring initiative: code quality, docs, testing, benchmarking, deps (proposals 1–7)#39
Open
haoyu-zc wants to merge 15 commits into
Open
Refactoring initiative: code quality, docs, testing, benchmarking, deps (proposals 1–7)#39haoyu-zc wants to merge 15 commits into
haoyu-zc wants to merge 15 commits into
Conversation
added 15 commits
July 16, 2026 14:28
Deep-review-driven improvement plan across code quality, docs, testing, benchmarking, dependencies, and optimization. Each change carries a proposal, design, delta specs, and task breakdown.
…ness) OpenSpec change: fix-cuda-correctness. Fixes correctness bugs found in the 2026-07-16 review, concentrated in the CUDA p-value permutation path. Error handling - utils.cuh: gpu_assert/CUDA_CHECK* now throw std::runtime_error (pybind11 -> Python RuntimeError) instead of exit(); new CUDA_CHECK_KERNEL checks launch + sync after every kernel. No exit() remains in library code. - metrics.cu: process_input_array validates dtype/ndim/shape against (n_features, n_parts, n_objs) and raises ValueError; underflow guard reordered. - coef.cu: compute_coef validates n_partitions in (0, 255]. P-value path correctness - 64-bit indexing (math.cuh get_coords_from_index overload; computePValues) so n_feature_comp * n_perms no longer wraps at 2^32. - long long pair-confusion sums in computePermutedARI (no int overflow at ~46k+ objects/cluster). - Removed the MAX_CLUSTERS=16 silent-zero cliff: per-thread global scratch sized by k supports arbitrary cluster counts. - Null distribution now uses the same invalid-partition semantics as the observed statistic (shared classify_partition_pair helper: categorical -1 -> 0.0, singleton -2 -> NaN, clamp >= 0); permute-target matches the CPU reference. - Memory-bounded batching of d_perm_ccc_values (chunked) so large inputs don't OOM. - findMaxAriKernel negative-key path no longer writes garbage into max_parts. Python - Fixed the inverted -1/-2 singleton/categorical legend in ccc() docstrings (impl_gpu.py, impl.py) to match get_parts and the code. Tests - New tests/gpu/test_cuda_correctness.py (9 tests): shape/partition validation, interpreter survival, >16-cluster p-values vs CPU, categorical & singleton p-value consistency vs CPU. Behavior changes: previously-silent failures now raise; p-values change for partitions with >16 clusters or categorical/singleton features (old values were biased/wrong; new values track the CPU reference).
The per-permutation contingency scratch scaled as n_perms * k^2 and was allocated in full before the memory-chunking loop, so the newly-supported high-cluster p-value path could still OOM (e.g. k=10000 -> ~120 GB). Process permutations in sub-batches sized to a memory budget (<=1 GiB); scratch is now indexed by the local sub-batch id, so memory is bounded regardless of k and large cluster counts simply take more passes. Results are unchanged (each permutation is independent).
OpenSpec change: cleanup-dead-code. Deletes upstream-fork leftovers and the GPU cut-over's dead code; slims the published wheel. Dead Python - impl_gpu.py: removed the unreachable CPU compute stack (cdist_parts_basic/ parallel, compute_ccc, compute_ccc_perms, compute_coef, get_coords_from_index), dead imports (unravel_index_2d, ari, DummyExecutor, as_completed), the dead X_has_cat_features local, and the commented debug-export block. Live ccc() GPU path (-> ccc_cuda_ext.compute_coef) untouched. - Deleted sklearn/metrics_gpu.py (abandoned cupy prototype), empty numpy/ subpackage, and the stray scaffold libs/ccc/pyproject.toml. Dead CUDA/C++ - Deleted cub.cu (demo), example_return_optional_vectors (+ decl + binding + its test), ari_reduced, the dead double s_mem_size add, orphan libs/ccc_cuda_ext/CMakeLists.txt, and the broken test stubs. - Documented return_parts as Python-side-only in the binder. Repo hygiene - Removed 29 committed benchmark .log files, the nested tests/gpu/tests/ dir, print-only pseudo-tests, the dead tests/gpu/excluded/ dir, and unused ccc-example-*.pkl fixtures; fixed tests/data/README.md naming. - .gitignore: ignore *.log, tests/**/logs/, build dirs, compile_commands.json. Docker / environment - Replaced the broken upstream Dockerfile (copied a nonexistent env file, no CUDA base) with a minimal nvidia/cuda:12.5.1-runtime + conda-lock image referencing only files that exist; deleted entrypoint.sh and upstream environment leftovers. Wheel scoping (BREAKING for wheel imports) - Excluded research-only modules (plots.py, methods.py, giant.py, corr.py) from the published wheel; they remain in-repo for analysis/. log.py/log_config.yaml kept (used by ccc.utils.curl and tests); added pyyaml to declared deps so every shipped module imports cleanly. See CHANGELOG.md. Tests: removed 4 tests that exercised the deleted demo binding; focused gate 214 passed, 2 deselected (pre-existing np.unicode_).
…ling) OpenSpec change: add-lint-tooling. Single-source lint/format config + CI. - pyproject.toml: recreate the ruff config (deleted with the scaffold in the cleanup PR) at the root as the single source; rule set B,I,ARG,C4,EM,PL,PT, PTH,RET,SIM,UP,NPY,PD (+E/F/W); documented ignore list splits formatter-owned codes from logic-adjacent codes deferred to follow-up; per-file-ignores for tests; analysis/ and docs/ excluded. - .pre-commit-config.yaml: drop black/black-jupyter (ruff-format replaces them), bump ruff to v0.15.1, add mirrors-clang-format hook scoped to the extension. - .clang-format (Microsoft/4-space/120/PointerAlignment Right/sorted includes) and .clang-tidy (curated bugprone/performance/modernize checks). - CMakeLists.txt: export compile_commands.json for clang-tidy. - .github/workflows/ci.yml: lint (ruff + clang-format check), build, and CPU-test jobs (build/test use a CUDA devel container; GPU tests remain local). - .git-blame-ignore-revs for the mechanical format commit.
Mechanical, tool-generated formatting of the package, tests, and CUDA extension (scope: libs/ccc, tests, libs/ccc_cuda_ext; analysis/ untouched). No behavior change intended. - Python: `ruff format` reflow + `ruff check --fix` safe fixes only (import sorting, unused-import removal in tests, Union->|, .format->f-string, redundant open mode, NPY201 np.unicode_->np.str_). - CUDA/C++: `clang-format -i` (Microsoft base, 4-space, 120 cols, sorted includes within each group; local headers unmoved). The NPY201 fix (np.unicode_->np.str_) in tests/test_coef_pval.py incidentally resolves the 2 pre-existing NumPy-2 test failures — they now pass.
…ture-tests) OpenSpec change: restructure-tests. - Markers gpu/slow/network registered with --strict-markers; tests/gpu/conftest.py auto-marks the GPU dir and skips cleanly when cupy/ccc_cuda_ext are absent, so `pytest tests/ -m "not gpu"` passes on a CPU-only box. tests/conftest.py holds shared fixtures; clean_gpu_memory is now an autouse fixture. - De-benchmarked the parity tests: removed the duplicated timing/logging/log-file machinery from test_ccc_gpu.py and test_ccc_gpu_return_parts.py (kept the assert_frame_equal parity at 1e-6), removed wall-clock speedup assertions from the n_jobs tests (now determinism checks), deleted the pure-timing benchmark tests (their intent moves to ccc-gpu-bench). - CUDA gtests wired into ctest behind option(CCC_BUILD_TESTS OFF); googletest FetchContent gated inside the option so default/wheel builds do not fetch it; registered via gtest_discover_tests. Two oversized reference cases GTEST_SKIP. - Closed GPU coverage gaps (all pass after the fix-cuda-correctness fixes): categorical return_parts parity, constant-feature matrix, too-few-objects error parity, mixed numerical+categorical DataFrame (tests/gpu/test_coverage_gaps.py). - Updated scripts/run_tests.sh, tests/README.md, and ci.yml to the marker-based commands. (pyproject.toml also carries the bench console-script for the companion extract-benchmarks commit.)
OpenSpec change: extract-benchmarks. Decouples performance measurement from the test suite into a standalone CLI (`ccc-gpu-bench` / `python -m ccc.bench`). - Modes: coef (GPU-vs-CPU end-to-end, --pvalue-n-perms/--return-parts/ --gpu-only/--cpu-only + grids), ari (kernel-level), scaling (CPU n_jobs). - Structured output: JSONL (default) or CSV, incremental writes, one record per case with full config, seed, env metadata (GPU/driver/CUDA/CPU/pkg version), and warmup-excluded repeated timings + speedup. - Presets smoke (seconds) and paper (poster/README grid). --profile gives a CPU cProfile category breakdown and prints an nsys suggestion for GPU runs. - Stdlib-only (argparse); graceful degradation without a GPU. CPU-only smoke test for the CLI plumbing (tests/test_bench_cli.py).
- All human-readable status/progress/nsys-suggestion output now goes to stderr (cli.py, runners.py), so the default JSONL/CSV written to stdout stays machine-parseable when --output is omitted or '-'. - gpu_available() now probes cupy for an accessible CUDA device count instead of only checking that cupy/ccc_cuda_ext are importable, so a device-less host (CUDA env on a CPU runner) fails fast with a clear message.
…encies) OpenSpec change: upgrade-dependencies. Aligns declared pins/config with the tested modern stack (numpy 2.x / numba / pandas 3 / pybind11 3 / py3.12 / CUDA 12.x) and broadens GPU support. - build-system: drop setuptools/wheel; add scikit-build-core>=0.10; pin pybind11>=2.13,<4 (verified building on 3.0.3). - CUDA architectures 75 -> 75;80;86;89;90 (native SASS for Turing..Hopper; verified via cuobjdump). Reconciled a single CMake floor (3.26...4.0) across pyproject and CMakeLists.txt. - Extras [project.optional-dependencies]: plots, research, test — so the core install stays lean and research modules install on demand. - plots.py: replaced the private seaborn._freedman_diaconis_bins import with a public reimplementation (numerically identical). - environment: environment-gpu.yml is now the real single lock source (py3.12/ numpy2/numba/CUDA12.x); folded benchmark/toolchain env files into extras; new environment/README.md documents the conda-lock regen command. - cibuildwheel: toolkit 12.5->12.8 (fixed the stale "12.9" comment), arch list, Python range 3.10-3.14.
OpenSpec change: improve-docs. - P-VALUE DOCS (the reported gap): rewrote the pvalue_n_perms docstring in impl.py and impl_gpu.py — one-sided permutation test, estimator (count+1)/(n_perms+1), interpretation, min resolvable p ~ 1/(n_perms+1), returned tuple shapes, and a cost warning for 2d inputs (a p-value per pair x n_perms extra evaluations). Fixed typos (cm_vlaues, pvalue_n_permutations, returns_parts, coordiates, maximimized). Added a "Computing p-values" section to usage.rst and expanded the README. - Fixed ccc() return-type annotations to the real polymorphic return. - Version single-sourcing: ccc.__version__ via importlib.metadata; conf.py reads package metadata; CITATION.cff -> 0.2.4. One version everywhere (0.2.4). - LICENSE: aligned CITATION.cff (was MIT), README, and pyproject classifier to the authoritative LICENSE file (BSD-2-Clause-Patent). SEE PR NOTE — confirm. - Sphinx: added an autodoc API page (api.rst) rendering ccc(); switched theme to sphinx_rtd_theme; filled the bindings.rst stub; de-duplicated PUBLISHING.md; added Marc Subirana-Granes to authors; corrected the stale citation in introduction.rst (Bioinformatics 2026, btag068). - Added a benchmarking.rst page for the ccc-gpu-bench CLI + README pointer. - Reconciled README/docs claims (real PyPI install, CUDA/CC floor, Python range).
…codex) Addresses the two codex findings on PR E: - [P1] conda-lock.yml still resolved Python 3.10 / NumPy 1.26 and referenced the removed environment-gpu-new.yml, so the documented `conda-lock install` built the OLD stack. Slimmed environment-gpu.yml to the reproducible build/test/docs env (Python 3.12, NumPy 2.x, numba, CUDA 12.5 toolkit, pybind11 3.x, sphinx), removing the research/analysis-only deps (minepy/seaborn/etc. — minepy has no modern-Python build and blocked the solve), and regenerated conda-lock.yml from scratch. The lock now matches the declared source. - [P2] Removed the `plots` and `research` optional-dependency extras: the modules they target (ccc.plots/methods/giant) are intentionally excluded from the wheel (cleanup change), so `pip install cccgpu[plots]` could never provide them. Their deps are documented for source-checkout/analysis use instead. Kept `test`.
…dex) --repeats 0 or negative previously crashed in _timed() with a confusing ValueError from min(times); negative --warmup was silently treated as 0. Add argparse type validators so invalid values fail cleanly before any work runs.
Collaborator
Author
|
Codex review of the cumulative diff found one [P2]: the |
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
1 participant
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.
Integration branch for the July 2026 repo-wide refactoring initiative. Bundles proposals 1–7 of the deep-review plan (each landed and reviewed as its own PR into this branch: #33, #34, #35, #36, #37). Proposal 8 (optimization backlog) is a separate PR (#38) into this branch, for review — intentionally not folded in here.
14 commits · 193 files · +8,183 / −9,745.
What's included
long longsums, removal of the silentMAX_CLUSTERS=16zero-cliff, consistent invalid-partition semantics for the permutation null, throwing error macros instead ofexit(), input-shape validation, and memory-bounded permutation batching. New GPU regression tests.cub.cu, demo bindings, orphan CMake, broken test stubs, 29 committed logs), replaces the broken Dockerfile, and slims the published wheel..git-blame-ignore-revs.gpu/slow/networkmarkers sopytest tests/is green on a CPU-only box, benchmarks pulled out of the test suite, CUDA gtests wired intoctest(gated so wheel builds stay offline), and the newccc-gpu-benchCLI.75;80;86;89;90), modern dependency pins, and a regenerated conda-lock for the Python 3.12 / NumPy 2.x stack.Testing
Built and verified in a dedicated conda env (Python 3.12, NumPy 2.4, CUDA 12.5, RTX 4090): 139 CPU + 79 GPU + 7 slow tests pass,
ctest8 pass / 2 skip,ccc-gpu-benchworks, docs build clean undersphinx-build -W. Every constituent PR was independently rebuilt, re-tested, and code-reviewed before merge into this branch.Two decisions to confirm before merging to main
LICENSEfile is BSD-2-Clause Plus Patent, butCITATION.cffpreviously said MIT. This branch aligns CITATION/README/pyproject to the LICENSE file. If MIT was intended, that should be flipped instead.Related