Skip to content

fix: use get_igraph_from_adjacency for graph construction#4112

Merged
flying-sheep merged 7 commits into
mainfrom
pa/graph-construction
May 19, 2026
Merged

fix: use get_igraph_from_adjacency for graph construction#4112
flying-sheep merged 7 commits into
mainfrom
pa/graph-construction

Conversation

@flying-sheep
Copy link
Copy Markdown
Member

@flying-sheep flying-sheep commented May 8, 2026

@flying-sheep flying-sheep added this to the 1.12.2 milestone May 8, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 8, 2026

Codecov Report

❌ Patch coverage is 86.36364% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.61%. Comparing base (bb4e605) to head (30bc534).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/scanpy/neighbors/__init__.py 0.00% 1 Missing ⚠️
src/scanpy/tools/_paga.py 0.00% 1 Missing ⚠️
src/scanpy/tools/_utils.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4112      +/-   ##
==========================================
- Coverage   79.61%   79.61%   -0.01%     
==========================================
  Files         120      120              
  Lines       12782    12786       +4     
==========================================
+ Hits        10176    10179       +3     
- Misses       2606     2607       +1     
Flag Coverage Δ
hatch-test.low-vers 78.85% <86.36%> (+<0.01%) ⬆️
hatch-test.pre 79.47% <86.36%> (+<0.01%) ⬆️

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

Files with missing lines Coverage Δ
src/scanpy/_settings/presets.py 82.81% <100.00%> (ø)
src/scanpy/_utils/__init__.py 75.34% <100.00%> (+0.22%) ⬆️
src/scanpy/metrics/_metrics.py 97.14% <100.00%> (-0.05%) ⬇️
src/scanpy/plotting/_tools/paga.py 64.26% <100.00%> (ø)
src/scanpy/tools/_draw_graph.py 69.01% <100.00%> (ø)
src/scanpy/neighbors/__init__.py 82.04% <0.00%> (ø)
src/scanpy/tools/_paga.py 32.05% <0.00%> (ø)
src/scanpy/tools/_utils.py 86.76% <85.71%> (+0.19%) ⬆️

... and 1 file with indirect coverage changes

@flying-sheep flying-sheep changed the title fix: use get_igraph_from_adjacency for graph construction for now. fix: fix modularity inconsistency by using get_igraph_from_adjacency for now. May 8, 2026
@flying-sheep flying-sheep changed the title fix: fix modularity inconsistency by using get_igraph_from_adjacency for now. fix: use get_igraph_from_adjacency for graph construction May 11, 2026
@flying-sheep flying-sheep marked this pull request as ready for review May 11, 2026 07:06
@flying-sheep flying-sheep requested a review from ilan-gold May 18, 2026 14:30
Copy link
Copy Markdown
Contributor

@ilan-gold ilan-gold left a comment

Choose a reason for hiding this comment

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

Is this a replacement for #4113?

Also should we add some documentation for this new behavior to the presets?

cc @jpintar

@flying-sheep
Copy link
Copy Markdown
Member Author

flying-sheep commented May 18, 2026

This one is backwards-compatible, #4113 needs some more work due to igraph not handling sparse arrays (only matrices).

@flying-sheep flying-sheep requested a review from ilan-gold May 19, 2026 10:39
Copy link
Copy Markdown
Contributor

@ilan-gold ilan-gold left a comment

Choose a reason for hiding this comment

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

Just documentation stuff. I think we should be very clear what is going on, why, and what the plan is

Comment thread src/scanpy/_utils/__init__.py
)
raise
igraph_mode: str = ig.ADJ_DIRECTED if is_directed else ig.ADJ_UNDIRECTED
graph: ig.Graph = ig.Graph.Weighted_Adjacency(connectivities, mode=igraph_mode)
Copy link
Copy Markdown
Contributor

@ilan-gold ilan-gold May 19, 2026

Choose a reason for hiding this comment

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

This is the change though right? Like users of modularity will start seeing a different number? And we're considering that a bug? Just want to be 100% clear. I would explain the rationale behind breaking this in the release note more

Copy link
Copy Markdown
Member Author

@flying-sheep flying-sheep May 19, 2026

Choose a reason for hiding this comment

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

Yes, and for scanpy 2.0 we’ll change it back again. Not 100% convinced with that back-and-forth, but changing it in leiden would mean to change its results or to create two graphs, one just for the modularity …

We could also not change this site and just merge the “in the future, do things like we do them here” part

Copy link
Copy Markdown
Contributor

@ilan-gold ilan-gold May 19, 2026

Choose a reason for hiding this comment

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

The path forward seems reasonable. I am fine with this. The above is a bug against our intention, to match leiden, and the way forward is to rectify an underlying bug with our intention, that leiden is using the wrong graph. The flip-flop is just the fact that we have two options in two places and made the wrong choice both times.

Comment thread tests/test_metrics.py
Comment thread src/scanpy/_utils/__init__.py
@flying-sheep flying-sheep enabled auto-merge (squash) May 19, 2026 17:52
@flying-sheep flying-sheep merged commit 6bfd839 into main May 19, 2026
14 checks passed
@flying-sheep flying-sheep deleted the pa/graph-construction branch May 19, 2026 18:01
@scverse scverse deleted a comment from lumberbot-app Bot May 20, 2026
flying-sheep added a commit that referenced this pull request May 20, 2026
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.

Inconsistency in graphs used by sc.tl.leiden and sc.metrics.modularity leads to modularity mismatch

2 participants