feat: add networkit flavor for parallel Leiden clustering - #4170
amalia-k510 wants to merge 14 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4170 +/- ##
=========================================
+ Coverage 0 81.75% +81.75%
=========================================
Files 0 134 +134
Lines 0 13067 +13067
=========================================
+ Hits 0 10683 +10683
- Misses 0 2384 +2384
Flags with carried forward coverage won't be shown. Click here to find out more.
|
ilan-gold
left a comment
ilan-gold
left a comment
There was a problem hiding this comment.
Nice! Would be best to run the installation check :)
Also please add an extra to our pyproject.toml.
Let's try to use a heuristic about warning users about large inputs to igraph and the faster alternative
| networkit.setSeed(seed, useThreadId=True) | ||
| # only undirected for Parallel Leiden | ||
| g = _utils.get_networkit_from_adjacency(adjacency, weighted=use_weights) | ||
| iterations = n_iterations if n_iterations > 0 else 3 |
There was a problem hiding this comment.
Where did the number 3 come from?
There was a problem hiding this comment.
I checked the the NetworKit's header file for the ParallelLeiden and found that default for ParallelLeiden.iterations is set to three; hence why I decided to use it here as well.
explicit ParallelLeiden(const Graph &graph, int iterations = 3, bool randomize = true, double gamma = 1);
There was a problem hiding this comment.
Can we document that? Also, why isn't it the default then for python?
There was a problem hiding this comment.
I checked and turns out that iterations=3 is already the default on the Python side for ParallelLeiden. I could drop it, but I left it explicit so the value is obvious at the call site and clearly maps to scanpy's n_iterations.
|
Also can you point this branch at networkit/networkit#1422 (and then |
| if importlib.util.find_spec("networkit") is None: | ||
| return |
There was a problem hiding this comment.
Would still warn even if there is no networkit installed :)
Do you have a benchmarking notebook as a gist somewhere?
There was a problem hiding this comment.
I think this is already handled. It returns early if find_spec("networkit") is None, so the hint only fires when networkit's actually installed. Might've been from before I pushed the guard? Let me know if you're still seeing it warn without networkit and I'll take another look.
There was a problem hiding this comment.
Also, I tried to create gist, and it is erroring out on the notebook. I think the file is too large with the embedded outputs. Want me to push it to the PR branch, strip the outputs and re-upload to a gist, or just share it another way?
There was a problem hiding this comment.
Discussed IRL, but notebook with a cell for downloading the data works
for more information, see https://pre-commit.ci
…into networkit_gve
amalia-k510
changed the title
for more information, see https://pre-commit.ci
ilan-gold
left a comment
ilan-gold
left a comment
There was a problem hiding this comment.
Nice! Just small stuff. Can you add your notebook as well? Really great to see this!
| else pytest.warns( | ||
| UserWarning, match=r"The `igraph` implementation of leiden clustering" | ||
| ) | ||
| ): |
There was a problem hiding this comment.
We should probably update this warning to use _maybe_suggest_networkit if networkit is detected
| @needs.leidenalg | ||
| @needs.igraph | ||
| def test_leiden_equal_defaults_same_args(adata_neighbors): | ||
| """Ensure the two implementations are the same for the same args.""" | ||
| leiden_alg_clustered = sc.tl.leiden( | ||
| adata_neighbors, flavor="leidenalg", copy=True, n_iterations=2 | ||
| ) | ||
| igraph_clustered = sc.tl.leiden( | ||
| adata_neighbors, flavor="igraph", copy=True, directed=False, n_iterations=2 | ||
| ) | ||
| assert ( | ||
| normalized_mutual_info_score( | ||
| leiden_alg_clustered.obs["leiden"], igraph_clustered.obs["leiden"] | ||
| ) | ||
| > 0.9 | ||
| ) | ||
|
|
||
|
|
||
| @needs.leidenalg | ||
| @needs.igraph | ||
| def test_leiden_equal_defaults(adata_neighbors): | ||
| """Ensure that the old leidenalg defaults are close enough to the current default outputs.""" | ||
| leiden_alg_clustered = sc.tl.leiden( | ||
| adata_neighbors, flavor="leidenalg", directed=True, copy=True | ||
| ) | ||
| igraph_clustered = sc.tl.leiden( | ||
| adata_neighbors, flavor="igraph", copy=True, n_iterations=2, directed=False | ||
| ) | ||
| assert ( | ||
| normalized_mutual_info_score( | ||
| leiden_alg_clustered.obs["leiden"], igraph_clustered.obs["leiden"] | ||
| ) | ||
| > 0.9 | ||
| ) |
There was a problem hiding this comment.
Can we add networkit variants to both of these tests? I would expect fairly concordant results.
| # Seeding controls which nodes are tried and in what order, but not | ||
| # which thread wins when two move neighboring nodes simultaneously. | ||
| # So runs are statistically reproducible for a fixed thread count, | ||
| # but not bit-for-bit identical. |
There was a problem hiding this comment.
Oh wow, this is an amazing find. Can we add a unit test for this maybe? I think I mentioned adding a unit test independently of this anyway. I would also mention this in the docstring.
2 participants
This PR adds NetworKit's ParallelLeiden as a new flavor option for
scanpy.tl.leiden, enabling multithreaded Leiden community detection viascanpy.tl.leiden(adata, flavor="networkit"). The motivation behind is that scanpy's current Leiden backends (igraph, leidenalg) are single-threaded. For atlas-scale datasets (500k+ cells), clustering becomes a bottleneck. NetworKit's ParallelLeiden is a C++ parallel implementation available via pip with no additional compilation, making it the lowest-friction path to parallel Leiden in scanpy.