diff --git a/src/graph/algorithms/leiden/partition.ts b/src/graph/algorithms/leiden/partition.ts index ad2464f5..69fd9208 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 ce3ef58a..698446bc 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 b02a077c..37f22e26 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]); + }); +});