feat(network): replace union-find stand-in with Leiden consensus - #351
feat(network): replace union-find stand-in with Leiden consensus#351seonghobae wants to merge 4 commits into
Conversation
GAP-009 remainder: Traag, Waltman, and van Eck (2019) Leiden modularity (γ = 1) now partitions each co-assignment replicate. Fast local moving, refinement that keeps communities well-connected, and aggregation replace greedy union-find. Two cliques joined by a weak bridge stay two communities. Isolated topics stay unclustered. The Monti (2003) / Hennig (2007) consensus wrapper is unchanged. Not a graphical lasso, not a causal cluster, and not an export workflow.
📝 WalkthroughWalkthrough
ChangesLeiden 합의 클러스터링
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to Replacing the previous clustering behavior changes consensus partitions, but the current implementation can incorrectly classify topics when edges reference invalid endpoints and can produce different consensus results when equivalent edges are reordered. These reproducibility and correctness risks should be fixed or explicitly accepted before merge; citation metadata in the accompanying research documentation also needs correction. Sequence Diagram(s)sequenceDiagram
participant consensus_clusters
participant leiden_partition
participant co_assignment
participant ClusterOutput
consensus_clusters->>leiden_partition: 각 replicate의 surviving edges 분할
leiden_partition->>co_assignment: replicate별 community labels 전달
co_assignment->>ClusterOutput: co-assignment threshold 적용
ClusterOutput-->>consensus_clusters: consensus clusters 반환
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 57.58% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 33 functions across 3 files. (5 skipped: 5 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let sub_total = sub_strength.iter().sum::<f64>() / 2.0; | ||
| if sub_total <= 0.0 { | ||
| continue; | ||
| } | ||
| let subgraph = Graph { | ||
| node_count: members.len(), | ||
| adjacency: sub_adj, | ||
| strength: sub_strength, | ||
| total_weight: sub_total, | ||
| }; | ||
| move_nodes_fast(&subgraph, &mut sub_membership, rng); |
There was a problem hiding this comment.
🔴 Refinement freezes improvable communities
When a parent has internal edges, refine_partition rescales modularity to its induced subgraph and can keep every node merged. Aggregation then hides profitable node moves, returning a lower-modularity partition.
Was this helpful? React with 👍 or 👎 to provide feedback.
| for node in 0..graph.node_count { | ||
| strength[refined[node]] += graph.strength[node]; | ||
| let source = refined[node]; | ||
| for &(neighbor, weight) in &graph.adjacency[node] { | ||
| let target = refined[neighbor]; | ||
| if source < target { | ||
| adjacency[source].push((target, weight)); | ||
| adjacency[target].push((source, weight)); | ||
| } | ||
| } | ||
| } | ||
| for neighbors in &mut adjacency { | ||
| neighbors.sort_by_key(|left| left.0); | ||
| neighbors.dedup_by(|later, kept| { | ||
| if later.0 == kept.0 { | ||
| kept.1 += later.1; | ||
| true | ||
| } else { | ||
| false | ||
| } | ||
| }); | ||
| } | ||
| let aggregate = Graph { | ||
| node_count: refined_count, | ||
| adjacency, | ||
| strength, | ||
| total_weight: graph.total_weight, |
There was a problem hiding this comment.
📝 Info: Aggregate self-loops are intentionally implicit
aggregate_graph omits internal edges but preserves their endpoint strengths and global weight. The current gain formula therefore retains their modularity contribution without explicit self-loops.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cargo fmt --check failed on network_analysis re-export order. The best_delta <= 0 stay-guard was unreachable once best_community left the source community, so nightly unique-branch coverage could not close. Source-out-of-range edges now exercise the other bound check.
…orts Aggregation never saw two edges between the same supernode pair, so the dedup merge arm stayed uncovered. Direct aggregate_graph recovery now sums parallel supernode weights and keeps a distinct neighbour. The connectedness oracle covers disconnected communities and singletons. Re-exports match rustfmt type-before-function order.
| best_community = candidate; | ||
| } | ||
| } | ||
| if best_community == from { |
There was a problem hiding this comment.
📝 Info: Removed guard was redundant
best_community changes only when delta exceeds the zero-initialized best_delta. Every accepted move therefore still has positive modularity gain.
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn aggregate_sums_parallel_supernode_edges_and_keeps_distinct_neighbors() { | ||
| // Two crossing edges between supernodes 0 and 1 plus one edge | ||
| // from 1 to 2. Aggregation must merge the parallel pair and keep | ||
| // the distinct neighbour. | ||
| let owned = vec![ | ||
| edge(0, 1, 1.0), | ||
| edge(2, 3, 1.0), | ||
| edge(4, 5, 1.0), | ||
| edge(0, 2, 0.3), | ||
| edge(1, 3, 0.4), | ||
| edge(2, 4, 0.2), | ||
| ]; | ||
| let edges = refs(&owned); | ||
| let graph = Graph::from_edges(&edges, 6); | ||
| let refined = vec![0, 0, 1, 1, 2, 2]; | ||
| let parent = refined.clone(); | ||
| let leaf_members: Vec<Vec<usize>> = (0..6).map(|node| vec![node]).collect(); | ||
| let (aggregate, next_membership, next_leaves) = | ||
| aggregate_graph(&graph, &parent, &refined, &leaf_members); | ||
| assert_eq!(aggregate.node_count, 3); | ||
| assert_eq!(aggregate.adjacency[0].len(), 1); | ||
| assert_eq!(aggregate.adjacency[0][0].0, 1); | ||
| assert!((aggregate.adjacency[0][0].1 - 0.7).abs() < 1e-15); | ||
| assert_eq!(aggregate.adjacency[1].len(), 2); | ||
| assert_eq!(aggregate.adjacency[1][0].0, 0); | ||
| assert_eq!(aggregate.adjacency[1][1].0, 2); | ||
| assert!((aggregate.adjacency[1][0].1 - 0.7).abs() < 1e-15); | ||
| assert!((aggregate.adjacency[1][1].1 - 0.2).abs() < 1e-15); | ||
| assert_eq!(next_membership, vec![0, 1, 2]); | ||
| assert_eq!(next_leaves[0], vec![0, 1]); | ||
| assert_eq!(next_leaves[1], vec![2, 3]); | ||
| assert_eq!(next_leaves[2], vec![4, 5]); | ||
| } |
There was a problem hiding this comment.
📝 Info: Aggregation coverage is structurally independent
The fixture directly constructs parallel aggregate edges and a distinct neighbor. Its assertions isolate edge summation and neighbor preservation from partition randomness.
Was this helpful? React with 👍 or 👎 to provide feedback.
| #[test] | ||
| fn connectedness_oracle_rejects_a_disconnected_community() { | ||
| let owned = vec![edge(0, 1, 1.0), edge(2, 3, 1.0)]; | ||
| let edges = refs(&owned); | ||
| assert!(!communities_connected(&edges, &[0, 0, 0, 0])); | ||
| assert!(communities_connected(&edges, &[0, 0, 1, 1])); | ||
| assert!(communities_connected(&edges, &[0, 1, 2, 3])); | ||
| } |
There was a problem hiding this comment.
📝 Info: Connectedness check has an independent control
The disconnected fixture forces the oracle’s rejection path. Separate connected-pair and singleton assignments prevent a vacuous always-false result.
Was this helpful? React with 👍 or 👎 to provide feedback.
The unique-branch gate failed 3949/3950 on the false arm of `!moved` when aggregation did not shrink. The stop predicate is now a named helper with both arms recovered (Traag, Waltman, & van Eck, 2019). Consensus drop sampling no longer shares the Leiden LCG; admitted edges are sorted by endpoint; out-of-range and self-loop endpoints stay unclustered. APA 7th Monti, Tamayo, Mesirov, and Golub (2003) coauthors and Hennig (2007) pages 258–271 are restored.
| && edge.source != edge.target | ||
| }) | ||
| .collect(); | ||
| admitted.sort_by_key(|edge| (edge.source, edge.target)); |
There was a problem hiding this comment.
🟡 Parallel-edge order changes consensus
When duplicate endpoint pairs carry different effects, sort_by_key preserves their input order and assigns fixed drop draws to different weights. Reordering equivalent edge lists can therefore change consensus clusters.
| admitted.sort_by_key(|edge| (edge.source, edge.target)); | |
| admitted.sort_by(|left, right| { | |
| left.source | |
| .cmp(&right.source) | |
| .then(left.target.cmp(&right.target)) | |
| .then_with(|| left.effect.total_cmp(&right.effect)) | |
| }); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| #[test] | ||
| fn aggregation_continues_when_nodes_moved_on_a_same_size_level() { | ||
| // Unique-branch coverage of the `!moved` false arm: refinement | ||
| // did not shrink the graph, but local moving still changed the | ||
| // partition, so Leiden must not stop (Traag et al., 2019). | ||
| assert!(aggregation_level_is_stable(4, 4, false)); | ||
| assert!(!aggregation_level_is_stable(4, 4, true)); | ||
| assert!(!aggregation_level_is_stable(2, 4, false)); | ||
| assert!(!aggregation_level_is_stable(1, 4, true)); | ||
| assert!(aggregation_level_is_stable(1, 1, false)); | ||
| assert!(!aggregation_level_is_stable(1, 1, true)); |
There was a problem hiding this comment.
🔍 Continuation path lacks integration coverage
The test invokes only aggregation_level_is_stable with synthetic inputs. It cannot detect failures reaching this branch or carrying its updated partition forward.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Hour-20 exact-head review request. Current head Draft GAP-009 Leiden. Do not convert a failing head into a second Leiden PR. Do not self-approve. Do not --admin merge. Checks/reviews are not a reason to weaken fail-closed gates. |
|
Hour-35 exact-head review request on draft Leiden. Current head Author/bot COMMENTED is not independent APPROVE. Copilot review requests are not APPROVE. Predecessor-head evidence is non-passing. Do not self-approve. @opencode-agent review |
|
Hour-36 re-verify: draft Leiden head 9945c91. Zero independent APPROVEs. Do not duplicate Leiden. Exact-head Checks on 9945c91 only. Do not self-approve. @opencode-agent review. |
1 participant
GAP-009 remainder on protected main
1bc02f580cf48e1d39da239f0e818453437c31c3. Head is9945c9147dcc266a1eaf6b984e7a341542c302c1.network_analysisnow runs Traag, Waltman, and van Eck (2019) Leiden modularity (γ = 1) on each co-assignment replicate instead of greedy union-find. Fast local moving, refinement that keeps communities internally connected, and aggregation are the partition. Two triangles joined by a weak bridge stay two communities; union-find glued them. Isolated topics stay unclustered. Aggregation sums parallel supernode edges and keeps distinct neighbours.Co-assignment follows Monti, Tamayo, Mesirov, and Golub (2003) and Hennig (2007) with an explicit
edge_drop_probability. The drop stream is independent of the per-replicate Leiden RNG; admitted edges are sorted by endpoint; out-of-range and self-loop endpoints stay unclustered. The aggregation stop predicate covers the continue-when-moved same-size arm.Not a graphical lasso, not a causal cluster, and not an export workflow. No Buyer language. Do not merge without two independent APPROVE reviews and exact-head Checks on this SHA. Predecessor-head evidence is non-passing.
Doctoring:
docs/research/leiden-consensus.md.