Skip to content

fix: copy connectivities before passing to umap-learn - #4031

Merged
flying-sheep merged 2 commits into
scverse:mainfrom
nbjustice:fix/umap-connectivities-copy-4028
Apr 7, 2026
Merged

fix: copy connectivities before passing to umap-learn#4031
flying-sheep merged 2 commits into
scverse:mainfrom
nbjustice:fix/umap-connectivities-copy-4028

Conversation

@nbjustice

Copy link
Copy Markdown
Contributor

Summary

sc.tl.umap() passes neighbors["connectivities"].tocoo() to umap-learn's simplicial_set_embedding() without copying. Since csr_matrix.tocoo() defaults to copy=False, the COO and CSR matrices share the same data buffer. umap-learn then zeroes out small edge weights in-place, which silently corrupts adata.obsp['connectivities'].

Fix: .tocoo().tocoo(copy=True)

Test plan

  • Added test_umap_preserves_connectivities asserting data values, nnz count, no explicit zeros, and embedding is still produced
  • Verified test fails without the fix and passes with it
  • All existing UMAP tests pass

@flying-sheep flying-sheep added this to the 1.12.1 milestone Apr 7, 2026
@flying-sheep

Copy link
Copy Markdown
Member

thank you, this looks wonderful!

@codecov

codecov Bot commented Apr 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.50%. Comparing base (1fbe008) to head (b446f41).
⚠️ Report is 74 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4031   +/-   ##
=======================================
  Coverage   78.50%   78.50%           
=======================================
  Files         117      117           
  Lines       12740    12740           
=======================================
  Hits        10001    10001           
  Misses       2739     2739           
Flag Coverage Δ
hatch-test.low-vers 77.79% <ø> (ø)
hatch-test.pre 77.45% <ø> (ø)

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

Files with missing lines Coverage Δ
src/scanpy/tools/_umap.py 72.30% <ø> (ø)

@flying-sheep
flying-sheep merged commit dbb5c58 into scverse:main Apr 7, 2026
14 checks passed
@scverse scverse deleted a comment from lumberbot-app Bot Apr 9, 2026
flying-sheep added a commit that referenced this pull request Apr 10, 2026
…assing to umap-learn) (#4047)

Co-authored-by: nbjustice <nbjustice@users.noreply.github.com>
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.

sc.tl.umap() silently mutates adata.obsp['connectivities'] via shared sparse buffer

2 participants