Record leiden/louvain modularity in adata.uns#648
Record leiden/louvain modularity in adata.uns#648jpintar wants to merge 4 commits intoscverse:mainfrom
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughLeiden and Louvain clustering functions now capture and record the modularity values returned by their underlying graph algorithms, storing them in ChangesModularity Capture and Persistence
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/rapids_singlecell/tools/_clustering.py (1)
279-281: ⚡ Quick winAdd
adata.unsassertions to the existing parametrised clustering test.The current test at
tests/test_clustering.py:73-83only checks that the correctadata.obscolumns exist. It does not verify the newadata.uns[key_added]["modularity"]and the updatedadata.uns[key_added]["params"]["resolution"]shape (scalar for a single resolution, list for multiple). Since these are the core deliverables of this PR, a missing assertion means a regression here would go undetected.✅ Suggested additions to the existing test
def test_clustering_resolution(adata_neighbors, clustering_function, resolution): adata = adata_neighbors.copy() clustering_function(adata, key_added="test_clustering", resolution=resolution) if isinstance(resolution, list): for r in resolution: assert f"test_clustering_{r}" in adata.obs.columns + # modularity and resolution should both be lists + assert isinstance(adata.uns["test_clustering"]["modularity"], list) + assert len(adata.uns["test_clustering"]["modularity"]) == len(resolution) + assert adata.uns["test_clustering"]["params"]["resolution"] == resolution else: assert "test_clustering" in adata.obs.columns + # modularity and resolution should both be scalars + assert isinstance(adata.uns["test_clustering"]["modularity"], float) + assert adata.uns["test_clustering"]["params"]["resolution"] == resolution🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rapids_singlecell/tools/_clustering.py` around lines 279 - 281, Add assertions in the parametrised clustering test to validate adata.uns entries created by the clustering function: assert that adata.uns[key_added]["modularity"] exists and equals modularities[0] when a single resolution was passed and equals the full modularities list when multiple resolutions were passed, and assert that adata.uns[key_added]["params"]["resolution"] is a scalar (single value) for single-resolution runs and a list (same length as resolutions) for multi-resolution runs; locate symbols key_added, modularities and adata.uns in the clustering function (e.g., where adata.uns[key_added]["modularity"] and adata.uns[key_added]["params"]["resolution"] are set) and add matching checks to the test to cover both single and multiple resolution parametrizations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/rapids_singlecell/tools/_clustering.py`:
- Around line 279-281: Add assertions in the parametrised clustering test to
validate adata.uns entries created by the clustering function: assert that
adata.uns[key_added]["modularity"] exists and equals modularities[0] when a
single resolution was passed and equals the full modularities list when multiple
resolutions were passed, and assert that
adata.uns[key_added]["params"]["resolution"] is a scalar (single value) for
single-resolution runs and a list (same length as resolutions) for
multi-resolution runs; locate symbols key_added, modularities and adata.uns in
the clustering function (e.g., where adata.uns[key_added]["modularity"] and
adata.uns[key_added]["params"]["resolution"] are set) and add matching checks to
the test to cover both single and multiple resolution parametrizations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4e8504fb-e185-4689-be19-4872e5682c6c
📒 Files selected for processing (1)
src/rapids_singlecell/tools/_clustering.py
|
Adding to @coderabbitai's note: Also worth checking: a release-notes entry in Skip this if the PR isn't flagged for the next dev release — no release note needed in that case. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1992
Under - {func}`~rapids_singlecell.tl.leiden` and {func}`~rapids_singlecell.tl.louvain` now record the final modularity value in `adata.uns[key_added]["modularity"]` (scalar for a single resolution, list for multiple resolutions) {pr}`648`.Under - `adata.uns[key_added]["params"]["resolution"]` is now stored as a scalar `float` when a single resolution is passed, and as a `list` when multiple resolutions are passed. Previously it was always stored as a list. {pr}`648`.The |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #648 +/- ##
==========================================
+ Coverage 88.03% 88.04% +0.01%
==========================================
Files 96 96
Lines 7035 7041 +6
==========================================
+ Hits 6193 6199 +6
Misses 842 842
|
3 participants
As of scverse/scanpy@9aaf126, Scanpy records the final modularity attained by
sc.tl.leideninadata.uns. This pull-request would add the same feature torsc.tl.leidenandrsc.tl.louvain.Additionally, for better consistency with Scanpy, which only allows a float rather than a list of floats for the
resolutionparameter, resolution and modularity are recorded as lists in.unsonly when a list of resolutions was passed, and as single floats otherwise.Closes #473.