fix: control loop values by daxpryce · Pull Request #71 · graspologic-org/graspologic-native

daxpryce · 2026-06-02T21:53:00Z

Control loop value flags that were added semantically differed from our intention. The delta has been addressed in this PR.

…ations Previously, max_outer_iterations used u32 internally with 0 as a sentinel for 'unlimited'. This meant max_outer_iterations=1 prevented all recursion (refinement + aggregation was skipped entirely), producing far too many communities on large graphs. Now uses Option<u32> internally: - None = unlimited recursion (converge naturally) - Some(0) = no recursion (local moving only) - Some(1) = one full cycle (LM → refine → aggregate → recurse once) - Some(n) = n full cycles allowed This matches the semantic convention of other Leiden implementations where max_iterations=1 means 'do one full outer Leiden loop including hierarchical aggregation', not 'skip aggregation entirely'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

When the induced network is not smaller than the current network (no aggregation progress), run one final local-moving pass on it without further recursion. This allows LM to recover from refinement splits (fixing small-graph tests) while preventing infinite oscillation. The inner aggregation still recurses to convergence when the network IS shrinking, and max_outer_iterations only controls the top-level outer loop — not recursion depth. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…avior Rewrite the top-level leiden() docstring to accurately document the outer-iteration cycle (LM → refine → aggregate → converge) and all parameters including max_outer_iterations and max_local_moving_iterations. Update internal function docstrings (improve_clustering, improve_clustering_view, improve_clustering_recursive) to precisely describe the convergence guard behavior: recurse when the induced network shrinks, one final LM pass otherwise. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

When the initial clustering for an induced network groups super-nodes by their pre-refinement cluster, and local moving has limited sweeps (max_local_moving_iterations), disconnected super-nodes can remain grouped together. Their subnetwork has no internal edges. Previously this case panicked with 'No node network'. Now it splits such clusters into singletons — the correct behavior since nodes with no internal edges should not be refined together. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

This PR updates the Leiden clustering implementation’s loop-control semantics and clarifies algorithm behavior (outer iterations vs. inner aggregation convergence), while also improving handling of edge cases during refinement/aggregation.

Changes:

Reworks “outer iteration” control to use a computed outer_limit and updates call sites accordingly.
Removes recursion-depth limiting logic in favor of converging inner aggregation based on network shrinkage, with a “final LM pass” fallback when shrinkage stalls.
Replaces panic-on-empty-subnetwork cases with a deterministic split-into-singletons behavior and updates related tests/docs.

Comments suppressed due to low confidence (2)

packages/network_partitions/src/leiden/leiden_clustering.rs:623

This test currently asserts max_outer_iterations=None matches Some(100), but Some(100) causes 100 outer iterations while None uses the iterations argument (currently Some(1)). That makes the comparison semantically inconsistent with the implementation and can make the test flaky/brittle.

        // max_outer_iterations = None should recurse until convergence
        let (_, clustering_none) = leiden(
            labeled_network.compact(),
            None,
            Some(1),

packages/network_partitions/src/leiden/leiden_clustering.rs:612

Test name/comment imply max_outer_iterations=None is “unlimited”, but the current implementation uses None to fall back to iterations, and uses Some(n) as the outer loop count override. The test name should reflect the aliasing behavior to avoid misleading future changes.

    fn test_max_outer_iterations_none_is_unlimited() {
        use rand::SeedableRng;
        use rand::rngs::SmallRng;

        let edges = edge_list();

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Rename test_max_outer_iterations_none_is_unlimited to test_max_outer_iterations_is_synonym_for_iterations — it now correctly verifies that iterations=3 and max_outer_iterations=Some(3) produce identical results (same seed, same loop count). - Rename test_max_outer_iterations_zero_prevents_recursion to test_max_outer_iterations_zero_is_noop — Some(0) means zero outer iterations (no work), not 'local moving only'. Assert the clustering is unchanged (all singletons) and improved=false. - Fix comment in test_max_outer_iterations_one_allows_single_recursion to reflect that Some(0) does nothing rather than 'local moving only'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

max_outer_iterations and iterations are distinct, orthogonal controls: - iterations: how many times to re-run the full Leiden cycle on the original network (outer loop), using the previous result as the starting clustering. - max_outer_iterations: within a single pass, how many levels of {LM → refine → aggregate} recursion are allowed. None/Some(0) means unlimited (converge). Some(1) means one aggregation level. Decrements on each recursion; stops when budget reaches 0 or 1. Also refactors improve_clustering to delegate to improve_clustering_recursive instead of duplicating 60+ lines of refinement/aggregation logic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

The max_outer_iterations parameter was added by mistake during debugging. It conflated two separate concepts: the outer retry loop (already handled by the existing 'iterations' parameter) and the inner aggregation recursion depth (which should always converge). Changes: - Remove max_outer_iterations from all Rust functions (leiden, leiden_view, improve_clustering, improve_clustering_view, improve_clustering_recursive) - Remove from hierarchical_leiden - Remove from Python bindings (leiden, hierarchical_leiden, leiden_csr) - Remove from mediator layer - Remove all max_outer_iterations-specific tests - Fix all interop test call sites (petgraph, sprs, csr_view) - Fix Python test call sites The algorithm now has two control knobs: - iterations: outer retry loop count (re-run full Leiden on original graph) - max_local_moving_iterations: node-visit sweep limit within local moving Inner aggregation recursion always converges (no depth limit), with a convergence guard to prevent infinite oscillation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

- iterations: clearly described as re-running the full Leiden cycle using the previous clustering as starting point - trials: clearly described as independent runs keeping the best - leiden_csr: expanded terse parameter descriptions for consistency - Return type: 'quality score' instead of 'modularity' (applies to both modularity and CPM modes) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

- Remove unused imports from lib.rs and mediator.rs - Remove crate-level #![allow(unused_imports)] suppression - Add trials >= 1 validation to non-CSR leiden mediator (matches leiden_csr which already had this check) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

daxpryce and others added 4 commits June 2, 2026 13:58

daxpryce requested a review from Copilot June 2, 2026 21:53

Copilot started reviewing on behalf of daxpryce June 2, 2026 21:53 View session

Copilot AI reviewed Jun 2, 2026

View reviewed changes

daxpryce and others added 2 commits June 2, 2026 15:03

style: cargo fmt

286c08a

daxpryce requested a review from Copilot June 2, 2026 22:14

Copilot started reviewing on behalf of daxpryce June 2, 2026 22:14 View session

Copilot AI reviewed Jun 2, 2026

View reviewed changes

Comment thread packages/network_partitions/src/leiden/leiden_clustering.rs Outdated

Comment thread packages/network_partitions/src/leiden/leiden_clustering.rs Outdated

daxpryce and others added 4 commits June 2, 2026 16:01

style: fix cli crate call site and cargo fmt

fa951e0

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

daxpryce requested a review from Copilot June 2, 2026 23:43

Copilot started reviewing on behalf of daxpryce June 2, 2026 23:44 View session

Copilot AI reviewed Jun 2, 2026

View reviewed changes

Comment thread packages/pyo3/src/mediator.rs

Comment thread packages/pyo3/src/lib.rs

Comment thread packages/pyo3/src/mediator.rs

daxpryce requested a review from Copilot June 3, 2026 00:02

Copilot started reviewing on behalf of daxpryce June 3, 2026 00:02 View session

Copilot AI reviewed Jun 3, 2026

View reviewed changes

Comment thread packages/network_partitions/src/leiden/leiden_clustering.rs

daxpryce merged commit 20f3920 into dev Jun 3, 2026
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: control loop values#71

fix: control loop values#71
daxpryce merged 11 commits into
devfrom
fix-control-loop-values

daxpryce commented Jun 2, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

daxpryce commented Jun 2, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants