Add CLAUDE.md maintainability checks and fix flagged code smells - #60
Merged
Merged
Conversation
Adapted from cccRISE's CLAUDE.md: adds vulture/radon/xenon to the dev dependency group and documents how to run dead-code, duplication, and cyclomatic-complexity checks against src/vsparse/. Running those checks surfaced and fixed: - _base.py: removed _VCSBase._major_range, a dead method with no callers anywhere in the repo (including tests). - _ivcsc.py/_rapid_load.py: extracted the repeated varint-decode loop into a shared _decode_varint njit helper, reused across _unpack, _decode_chunks, and _decode_selected_rows. - _anndata_class.py: extracted the duplicated X/raw_X coercion logic in VCSCAnnData's setters into a shared _coerce_vcs helper. The remaining jscpd-flagged duplication is the major=row/major=col pair of numba kernels repeated throughout _norm_common.py, _vcs_matmul.py, and _ops.py -- an intentional, documented pattern for cache-friendly parallel iteration over each array's own memory layout, not a code smell. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Pulls apart every plain-Python function radon/xenon ranked C (cyclomatic complexity >= 11), all in _anndata_class.py/_norm_common.py/_rapid_load.py, by extracting cohesive pieces into named helpers: - _anndata_class.py: __getitem__/copy/from_anndata/to_anndata/_write_group shared a repeated "filter the None key, transform each value" comprehension -- factored into _filtered/_map_subset_1d/_map_subset_2d/_map_copy. normalized()'s "reuse stats stored in obs/varm/uns" branch is now _normalized_from_stored(). X/raw_X's setter coercion was already shared via _coerce_vcs (see previous commit). - _norm_common.py: NormalizedViewBase.__init__'s VCSC/VCSR statistics dispatch and mean/scale finishing are now _column_gstats/_column_mean_scale. - _rapid_load.py: load_and_normalize's obs_filter/no-filter branches (each with their own gene-mask logic) are now _rows_without_obs_filter/ _rows_with_obs_filter/_compute_gene_mask; the h5py group read and the min_cells validation are now _read_ivcsr_group/_validate_min_cells. All of these were plain Python orchestration -- no behavior changes, and no numba kernels touched. The one function still ranked C, _column_stats_major_is_col in _norm_common.py, is a numba-parallel kernel whose branchiness is the documented, intentional cost of fusing three statistics passes into one over each column (see its docstring) -- left alone, consistent with CLAUDE.md's guidance not to force-fix pre-existing complexity that reflects a deliberate performance tradeoff. uv run pytest: 1352 passed, 61 skipped (unchanged). ruff/vulture/jscpd results unchanged from the previous commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016wKWZn2btQ3Uo4grjuxxd2
- benchmarks/cases.py: misaligned_matmat_vs_scipy/misaligned_rmatmat_vs_scipy still called peak_alloc_mb() after #56 dropped that helper entirely from benchmarks/harness.py, so every benchmark run failed with NameError. This was already broken on main (confirmed: main's own CI is red for the same reason), not something introduced by this branch, but it blocks this PR's gate too. Drops the two dead peak_alloc_mb entries (case output and baselines.json), matching how #56 already removed the other five memory-metric cases. - _ivcsc.py/_rapid_load.py: the shared _decode_varint helper from an earlier commit here expects a np.int64 position, but both callers seeded `pos` as a plain `0`, which `ty check` flagged as a real (if numba-tolerated) type mismatch. Seed `pos = np.int64(0)` instead. - _rapid_load.py: the _read_ivcsr_group extraction from the same earlier commit gives `kwargs` an explicit `dict[str, Any]` return type, which resolves the invalid-argument-type ty previously needed ignore comments for on `ad.AnnData(**kwargs)` -- removed the two now-unused ignores. - ruff format on the three files touched by the complexity refactor. uv run pytest: 1352 passed, 61 skipped. uv run python -m benchmarks.run --set fast: no regressions. ruff check/format, ty check, codespell all pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016wKWZn2btQ3Uo4grjuxxd2
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.
Summary
src/vsparse/), addingvulture,radon, andxenonto the dev dependency group and documenting how/when to run them._VCSBase._major_rangein_base.py— a dead method with no callers anywhere in the repo, including tests._ivcsc.py/_rapid_load.py) into a shared_decode_varintnumba helper, reused by_unpack,_decode_chunks, and_decode_selected_rows.X/raw_Xcoercion logic inVCSCAnnData's setters (_anndata_class.py) into a shared_coerce_vcshelper._norm_common.py,_vcs_matmul.py, and_ops.py— this is an intentional, already-documented pattern for cache-friendly parallel iteration over each array's own memory layout, not a code smell, so it's left alone (consistent with CLAUDE.md's own guidance to use judgment on tools like these rather than blindly act on every flag).VCSCAnnData.from_anndata,.normalized,.to_anndata,.__getitem__,.copy,load_and_normalize,_column_stats_major_is_col,NormalizedViewBase.__init__) were left as-is per CLAUDE.md's guidance, since none of them were touched by this change.Test plan
uv run pytest -q— 1352 passed, 61 skippeduv run ruff check src/vsparse/— all checks passeduv run vulture src/vsparse/— remaining hits confirmed as false positives (public API exercised only from tests)npx jscpd src/vsparse/ --min-lines 5 --min-tokens 50— duplicated clones down from 17 to 14🤖 Generated with Claude Code