From b4793180d27dcd5033f58e3e3e4946983c7524d7 Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Mon, 6 Jul 2026 04:48:30 -0600 Subject: [PATCH] refactor: reduce cyclomatic complexity of computeDeltaModularityDirected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit computeDeltaModularityDirected exceeded the cyclomatic threshold (11 vs warn=10) from four repeated "newC < arr.length ? fget(arr, newC) [|| 0] : 0" bounds-checked reads. Two of the four (inFromNew/outToNew, plus the oldC siblings inFromOld/outToOld) included a `|| 0` fallback; the other two (totalInStrengthNew/totalOutStrengthNew, plus totalInStrengthOld/ totalOutStrengthOld) did not. Traced the asymmetry to its root: all six arrays involved are dense, zero-initialized Float64Arrays populated purely by +=/-= over edge weights and node strengths that are scrubbed of NaN/undefined at adapter.ts's ingestion point (`+linkWeight(attrs) || 0`). No code path can put NaN or a sparse "hole" into any of them, so `|| 0` is a no-op today for both array families. The `|| 0` presence instead traces back to which arrays already had a getter-based public API: getNeighborEdgeWeightToCommunity/ getOutEdgeWeightToCommunity/getInEdgeWeightFromCommunity always bake in `|| 0`, and this function hand-inlined that same convention for the edge-weight arrays it reads directly, but never established an equivalent convention for the community-strength-total arrays (which is itself inconsistently applied elsewhere in this file, e.g. computeDeltaCPM's sizeOld vs. sizeNew on the same communityTotalSize array) — confirmed by diffing against the original vendored ngraph.leiden source, where this exact asymmetry already existed unchanged since the initial vendoring commit. Since the two families are provably equivalent here, extracted a single shared `fgetOrZero(arr, i)` helper (bounds check + `|| 0`) into typed-array-helpers.ts and applied it uniformly to all eight reads (newC/oldC x edge-weight x strength-total), documenting why the fallback is currently redundant but retained for defense-in-depth and consistency. Split the two extracted read-groups into computeDirectedEdgeWeightTerms/ computeDirectedStrengthTerms helper functions. cyclomatic 11 -> 3, cognitive 10 -> 2 for computeDeltaModularityDirected; new helpers are cyclomatic 1 (computeDirectedEdgeWeightTerms/ computeDirectedStrengthTerms) and 3 (fgetOrZero) — none exceed threshold. Verified behavior preservation two ways: - Direct detectClusters(graph, { directed: true }) comparison across 12 seed/resolution/file-vs-function-level combinations on this repo's own dependency graph (701 file nodes / 8833 function nodes) between the pre-refactor and post-refactor build: byte-for-byte identical quality() and community assignments. - codegraph communities -T --json (undirected path) before/after, controlling for native-vs-JS engine selection: byte-for-byte identical. Added unit tests for fgetOrZero's bounds/zero/NaN-squashing contract, plus two exact-value regression tests pinning computeDeltaModularityDirected's quality()/assignment output for the existing directed-modularity fixtures, so a future accidental re-divergence of the `|| 0` handling would be caught. Fixes #1755 docs check acknowledged Impact: 4 functions changed, 7 affected --- src/graph/algorithms/leiden/partition.ts | 61 +++++++++++--- .../algorithms/leiden/typed-array-helpers.ts | 27 +++++++ tests/graph/algorithms/leiden.test.ts | 80 +++++++++++++++++++ 3 files changed, 155 insertions(+), 13 deletions(-) diff --git a/src/graph/algorithms/leiden/partition.ts b/src/graph/algorithms/leiden/partition.ts index ad2464f5c..69fd92086 100644 --- a/src/graph/algorithms/leiden/partition.ts +++ b/src/graph/algorithms/leiden/partition.ts @@ -9,7 +9,7 @@ import type { GraphAdapter } from './adapter.js'; import { accumulateInternalEdgeWeights, accumulateNodeAggregates } from './aggregate-helpers.js'; -import { fget, iget, u8get } from './typed-array-helpers.js'; +import { fget, fgetOrZero, iget, u8get } from './typed-array-helpers.js'; export interface CompactOptions { keepOldOrder?: boolean; @@ -260,6 +260,46 @@ function computeDeltaModularityUndirected( return gain_remove + gain_add; } +/** + * Directed delta-modularity edge-weight terms: in/out edge weight from `v` to + * `newC`/`oldC`, read via the shared bounds-checked `fgetOrZero` (see its + * doc comment for why `|| 0` is safe/redundant here but retained anyway). + */ +function computeDirectedEdgeWeightTerms( + s: PartitionState, + newC: number, + oldC: number, +): { inFromNew: number; outToNew: number; inFromOld: number; outToOld: number } { + return { + inFromNew: fgetOrZero(s.inEdgeWeightFromCommunity, newC), + outToNew: fgetOrZero(s.outEdgeWeightToCommunity, newC), + inFromOld: fgetOrZero(s.inEdgeWeightFromCommunity, oldC), + outToOld: fgetOrZero(s.outEdgeWeightToCommunity, oldC), + }; +} + +/** + * Directed delta-modularity community-strength terms: total in/out strength + * of `newC`/`oldC`, read via the shared bounds-checked `fgetOrZero`. + */ +function computeDirectedStrengthTerms( + s: PartitionState, + newC: number, + oldC: number, +): { + totalInStrengthNew: number; + totalOutStrengthNew: number; + totalInStrengthOld: number; + totalOutStrengthOld: number; +} { + return { + totalInStrengthNew: fgetOrZero(s.communityTotalInStrength, newC), + totalOutStrengthNew: fgetOrZero(s.communityTotalOutStrength, newC), + totalInStrengthOld: fgetOrZero(s.communityTotalInStrength, oldC), + totalOutStrengthOld: fgetOrZero(s.communityTotalOutStrength, oldC), + }; +} + function computeDeltaModularityDirected( s: PartitionState, v: number, @@ -271,18 +311,13 @@ function computeDeltaModularityDirected( const totalEdgeWeight: number = s.graph.totalWeight; const strengthOutV: number = fget(s.graph.strengthOut, v); const strengthInV: number = fget(s.graph.strengthIn, v); - const inFromNew: number = - newC < s.inEdgeWeightFromCommunity.length ? fget(s.inEdgeWeightFromCommunity, newC) || 0 : 0; - const outToNew: number = - newC < s.outEdgeWeightToCommunity.length ? fget(s.outEdgeWeightToCommunity, newC) || 0 : 0; - const inFromOld: number = fget(s.inEdgeWeightFromCommunity, oldC) || 0; - const outToOld: number = fget(s.outEdgeWeightToCommunity, oldC) || 0; - const totalInStrengthNew: number = - newC < s.communityTotalInStrength.length ? fget(s.communityTotalInStrength, newC) : 0; - const totalOutStrengthNew: number = - newC < s.communityTotalOutStrength.length ? fget(s.communityTotalOutStrength, newC) : 0; - const totalInStrengthOld: number = fget(s.communityTotalInStrength, oldC); - const totalOutStrengthOld: number = fget(s.communityTotalOutStrength, oldC); + const { inFromNew, outToNew, inFromOld, outToOld } = computeDirectedEdgeWeightTerms( + s, + newC, + oldC, + ); + const { totalInStrengthNew, totalOutStrengthNew, totalInStrengthOld, totalOutStrengthOld } = + computeDirectedStrengthTerms(s, newC, oldC); // Self-loop correction + constant term (see modularity.ts diffModularityDirected) const selfW: number = fget(s.graph.selfLoop, v) || 0; const deltaInternal: number = diff --git a/src/graph/algorithms/leiden/typed-array-helpers.ts b/src/graph/algorithms/leiden/typed-array-helpers.ts index ce3ef58a4..698446bc9 100644 --- a/src/graph/algorithms/leiden/typed-array-helpers.ts +++ b/src/graph/algorithms/leiden/typed-array-helpers.ts @@ -22,6 +22,33 @@ export function u8get(a: Uint8Array, i: number): number { return a[i] as number; } +/** + * Bounds-checked community-accumulator read: `i < a.length ? (fget(a, i) || 0) : 0`. + * + * Community ids are sometimes probed before the community itself has been + * grown into a given per-community accumulator array (e.g. a brand-new + * `newC` about to receive its first member) — the bounds check treats that + * as a not-yet-existing community contributing zero, matching + * `getNeighborEdgeWeightToCommunity`/`getOutEdgeWeightToCommunity`/ + * `getInEdgeWeightFromCommunity` in partition.ts, which every other reader + * of these arrays already goes through. + * + * The `|| 0` is not reachable today: every array this is used with in this + * codebase (communityTotalStrength/In/Out, neighborEdgeWeightToCommunity, + * outEdgeWeightToCommunity, inEdgeWeightFromCommunity) is a dense, + * zero-initialized Float64Array populated purely by +=/-= over edge weights + * and node strengths that are scrubbed of NaN/undefined at + * `makeGraphAdapter` construction time (`+linkWeight(attrs) || 0` / + * `+nodeSize(attrs) || 0` in adapter.ts) — so a bare bounds-checked `fget` + * would return the identical value. Kept for defense-in-depth against a + * future change to that invariant, and so callers reading either array + * family use one consistent, already-safe accessor instead of + * hand-rolling the same ternary+`||` per call site (see issue #1755). + */ +export function fgetOrZero(a: Float64Array, i: number): number { + return i < a.length ? fget(a, i) || 0 : 0; +} + /** In-place compound addition: `a[i] += v`, safe under noUncheckedIndexedAccess. */ export function taAdd(a: Float64Array, i: number, v: number): void { a[i] = fget(a, i) + v; diff --git a/tests/graph/algorithms/leiden.test.ts b/tests/graph/algorithms/leiden.test.ts index b02a077c6..37f22e263 100644 --- a/tests/graph/algorithms/leiden.test.ts +++ b/tests/graph/algorithms/leiden.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from 'vitest'; import { detectClusters } from '../../../src/graph/algorithms/leiden/index.js'; +import { fgetOrZero } from '../../../src/graph/algorithms/leiden/typed-array-helpers.js'; import { CodeGraph } from '../../../src/graph/model.js'; // ─── Helpers ────────────────────────────────────────────────────────── @@ -651,3 +652,82 @@ describe('community connectivity', () => { } }); }); + +// ─── fgetOrZero (typed-array-helpers) ────────────────────────────────── +// +// Shared bounds-checked accessor extracted from computeDeltaModularityDirected +// (issue #1755) to unify the "newC < arr.length ? fget(arr, newC) [|| 0] : 0" +// pattern that previously appeared with an inconsistent `|| 0` across the +// edge-weight-to-community arrays vs. the community-strength-total arrays. + +describe('fgetOrZero (typed-array-helpers)', () => { + it('returns the stored value for an in-bounds index', () => { + const a = new Float64Array([1, 2, 3]); + expect(fgetOrZero(a, 1)).toBe(2); + }); + + it('returns 0 for an index at or beyond the array length (not-yet-existing community)', () => { + const a = new Float64Array([1, 2, 3]); + expect(fgetOrZero(a, 3)).toBe(0); + expect(fgetOrZero(a, 100)).toBe(0); + }); + + it('returns a plain 0 for a legitimate zero entry, same as a bare bounds-checked read', () => { + const a = new Float64Array([0, 5]); + expect(fgetOrZero(a, 0)).toBe(0); + }); + + it('squashes a stray NaN to 0 (defense-in-depth guard; not reachable via current callers)', () => { + const a = new Float64Array([Number.NaN, 5]); + expect(Number.isNaN(fgetOrZero(a, 0))).toBe(false); + expect(fgetOrZero(a, 0)).toBe(0); + }); +}); + +// ─── Directed modularity delta — exact regression values ─────────────── +// +// Pins the exact quality()/community-assignment output of the two existing +// "directed modularity" / "directed self-loops" fixtures above, computed +// through computeDeltaModularityDirected's fgetOrZero-based reads (issue +// #1755). These exact values were verified byte-for-byte identical against +// the pre-refactor implementation (four independent bounds-check-and-`||0` +// expressions) on this same seed. A future change that re-diverges the +// `|| 0` handling between the edge-weight and community-strength array +// families (or otherwise perturbs the delta-modularity formula) would shift +// these floating-point values and fail this test. + +describe('directed modularity delta — exact regression', () => { + it('matches the pinned quality/assignment for the two-triangle directed graph', () => { + const g = new CodeGraph(); + const A = ['0', '1', '2']; + const B = ['3', '4', '5']; + for (const id of [...A, ...B]) g.addNode(id); + for (let i = 0; i < A.length; i++) + for (let j = 0; j < A.length; j++) if (i !== j) g.addEdge(A[i], A[j]); + for (let i = 0; i < B.length; i++) + for (let j = 0; j < B.length; j++) if (i !== j) g.addEdge(B[i], B[j]); + g.addEdge('2', '3'); + + const clusters = detectClusters(g, { directed: true, randomSeed: 2 }); + expect(clusters.quality()).toBe(0.42603550295857995); + expect([...A, ...B].map((id) => clusters.getClass(id))).toEqual([1, 1, 1, 0, 0, 0]); + }); + + it('matches the pinned quality/assignment for the directed graph with self-loops', () => { + const g = new CodeGraph(); + const A = ['0', '1', '2']; + const B = ['3', '4', '5']; + for (const id of [...A, ...B]) g.addNode(id); + for (let i = 0; i < A.length; i++) + for (let j = 0; j < A.length; j++) if (i !== j) g.addEdge(A[i], A[j]); + for (let i = 0; i < B.length; i++) + for (let j = 0; j < B.length; j++) if (i !== j) g.addEdge(B[i], B[j]); + g.addEdge('2', '3'); + g.addEdge('0', '0', { weight: 3 }); + g.addEdge('3', '3', { weight: 3 }); + + const clusters = detectClusters(g, { directed: true, randomSeed: 2 }); + expect(clusters.quality()).toBe(0.44875346260387805); + expect([...A, ...B].map((id) => clusters.getClass(id))).toEqual([1, 1, 1, 0, 0, 0]); + }); +});