exp-algo: Louvain community detection (experimental)#67
exp-algo: Louvain community detection (experimental)#67shahar-biron wants to merge 4 commits intomainfrom
Conversation
Grant checks/pull-requests write permissions and force jest-coverage-report-action to run Jest in-band. Co-Authored-By: Warp <agent@warp.dev>
Introduce flex.exp.louvain (experimental) and an integration test demonstrating community detection on a weighted graph. Co-Authored-By: Warp <agent@warp.dev>
Add documentation for flex.exp.louvain and clearly mark exp-algo functions as experimental. Co-Authored-By: Warp <agent@warp.dev>
📝 WalkthroughWalkthroughAdds an experimental Louvain community-detection feature (implementation, docs, and integration tests) and updates CI to force single-threaded Jest runs and grant write permissions for checks and pull-requests. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
Coverage report
Show new covered files 🐣
Test suite run success79 tests passing in 27 suites. Report generated by 🧪jest coverage report action from 549c075 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/exp-algo/louvain.js`:
- Around line 118-165: The code currently accumulates weights into
undirectedWeightByKey (using undirectedWeightByKey.set(key, (get||0) + w)),
causing duplicate counting when the same edge is seen from both traversal
directions; change this to deduplicate by using a first-write-wins (or Math.max)
strategy: when computing the canonical key via makeUndirectedEdgeKey and
updating undirectedWeightByKey, set the value only if the key is not already
present (or replace with Math.max(existing, w)); apply the same change to the
fallback edge-handling path that also writes to undirectedWeightByKey, and leave
selfLoopWeightById behavior unchanged for true self-loops.
🧹 Nitpick comments (4)
.github/workflows/test.yml (1)
33-54: Tests run twice with different parallelism settings — first run may still be flaky.Line 36 runs
npm test -- --coverage(likely with Jest's default parallel workers), while the coverage-report action at Line 49 re-runs tests with--runInBand. If the flakiness stems from parallel execution against a single FalkorDB instance, the first run (Line 36) is still susceptible. Consider adding--runInBandto the first run as well, or consolidating into a single test execution that both checks coverage thresholds and feeds the report action.Proposed fix: make the first run consistent
- name: Run Tests with Coverage # Because coverageThreshold is in jest.config.js, # this step will fail if coverage is < 90% - run: npm test -- --coverage + run: npm test -- --coverage --runInBand --forceExit env: DEBUG: "true"src/exp-algo/louvain.js (1)
194-207: Edge-key split on'::'is fragile if node IDs ever contain that substring.
makeUndirectedEdgeKeyjoins with'::', and this code splits on'::'to recover endpoints. If a node ID is a string containing'::', the split will produce the wrong parts. Currently safe with integer FalkorDB IDs, but worth a defensive note or a separator that cannot appear in stringified IDs (e.g., a NUL character, or storing endpoints separately).docs/README.md (1)
108-116: Experimental section is well-documented with appropriate warnings.One minor omission: the "Browse by category" section (around Line 122–128) doesn't include a link for Experimental Graph Algorithms, unlike every other category. Consider adding one for discoverability:
- [Bitwise Functions](./docs/README.md#bitwise-functions-flexbitwise) - Low-level bit operations +- [Experimental Graph Algorithms](./docs/README.md#experimental-graph-algorithms-flexexp) - Community detection and more(in the root
README.md)tests/exp-algo/louvain.test.js (1)
27-136: Solid integration test with clear community structure.The 3-team graph with strong intra-team (weight=10) and weak cross-team (weight=1) edges is a good realistic scenario. The validation logic correctly treats community IDs as opaque and checks membership by name.
One observation: this only exercises
direction: 'incoming'. Consider adding a small test with the defaultdirection(both incoming and outgoing) to catch regressions related to bidirectional traversal — especially given the weight-accumulation behavior flagged in the implementation review.
| // First collect undirected edges into a canonical key -> weight map. | ||
| // This avoids double-counting when the same connection is observed from | ||
| // multiple traversal directions / endpoints. | ||
| const undirectedWeightByKey = new Map(); | ||
| const selfLoopWeightById = new Map(); | ||
|
|
||
| const debugEdgeCounts = debug ? Object.create(null) : null; | ||
|
|
||
| const directions = Array.isArray(direction) ? direction : [direction]; | ||
|
|
||
| for (const dir of directions) { | ||
| for (let i = 0; i < nodes.length; i++) { | ||
| const current = nodes[i]; | ||
| const currentId = getNodeId(current); | ||
| if (typeof currentId === 'undefined') continue; | ||
|
|
||
| const reachables = | ||
| graph.traverse([current], { | ||
| direction: dir, | ||
| returnType: 'edges', | ||
| }) || []; | ||
|
|
||
| const edges = reachables[0] || []; | ||
| if (debugEdgeCounts) { | ||
| debugEdgeCounts[String(currentId)] = edges.length; | ||
| } | ||
| const edgeCount = Math.min(edges.length, maxEdgesPerNode); | ||
|
|
||
| for (let e = 0; e < edgeCount; e++) { | ||
| const edge = edges[e]; | ||
| const w = getWeight(edge); | ||
| if (!(w > 0)) continue; | ||
|
|
||
| const s = edge ? edge.source : undefined; | ||
| const d = edge ? edge.destination : undefined; | ||
| const sId = s != null ? getNodeId(s) : undefined; | ||
| const dId = d != null ? getNodeId(d) : undefined; | ||
|
|
||
| // Preferred path: use both edge endpoints directly. | ||
| if (typeof sId !== 'undefined' && sId !== null && typeof dId !== 'undefined' && dId !== null) { | ||
| if (sId === dId) { | ||
| selfLoopWeightById.set(sId, (selfLoopWeightById.get(sId) || 0) + w); | ||
| continue; | ||
| } | ||
| const key = makeUndirectedEdgeKey(sId, dId); | ||
| undirectedWeightByKey.set(key, (undirectedWeightByKey.get(key) || 0) + w); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Edge weights are accumulated (not deduplicated) across traversal directions — the comment is misleading.
When direction = ['incoming', 'outgoing'] (the default), the same physical edge A→B is discovered once from A's outgoing traversal and once from B's incoming traversal. Both produce the same canonical key, but since the weight is summed ((get || 0) + w), the effective weight doubles.
For a uniform graph this doesn't affect community assignments (the modularity gain formula scales linearly), but if some edges are only visible from one direction (e.g., an endpoint is outside the nodes list), scaling becomes non-uniform and can distort results.
Options:
- Use
Math.max(or a first-write-wins guard) instead of accumulation to truly deduplicate. - Keep accumulation but fix the comment and document that the default bidirectional traversal intentionally doubles weights.
Option 1: deduplicate with first-write-wins
const key = makeUndirectedEdgeKey(sId, dId);
- undirectedWeightByKey.set(key, (undirectedWeightByKey.get(key) || 0) + w);
+ if (!undirectedWeightByKey.has(key)) {
+ undirectedWeightByKey.set(key, w);
+ }
continue;Apply the same pattern to the fallback path at Line 178.
🤖 Prompt for AI Agents
In `@src/exp-algo/louvain.js` around lines 118 - 165, The code currently
accumulates weights into undirectedWeightByKey (using
undirectedWeightByKey.set(key, (get||0) + w)), causing duplicate counting when
the same edge is seen from both traversal directions; change this to deduplicate
by using a first-write-wins (or Math.max) strategy: when computing the canonical
key via makeUndirectedEdgeKey and updating undirectedWeightByKey, set the value
only if the key is not already present (or replace with Math.max(existing, w));
apply the same change to the fallback edge-handling path that also writes to
undirectedWeightByKey, and leave selfLoopWeightById behavior unchanged for true
self-loops.
Add a long-running Louvain scaling test (12 * scale nodes; scale=1000 => 12k nodes) gated behind FLEX_LONG_TESTS=1. Co-Authored-By: Warp <agent@warp.dev>
1 participant
Adds experimental Louvain community detection under
flex.exp.louvain.Highlights:
flex.exp.louvain({nodes: collect(n), ...})weightrelationship property (default 1)Also includes CI stability fixes for the existing integration test suite:
checks: write+pull-requests: writepermissionsjest-coverage-report-actionto run Jest in-band to avoid flakinessCo-Authored-By: Warp agent@warp.dev
Summary by CodeRabbit
New Features
Documentation
Tests
Chores