Skip to content

refactor: remove unreachable Partition delta-computation interface methods #1896

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
carlos-alm wants to merge 1 commit into
base: fix/issue-1769-codegraph-method-attribution
Choose a base branch
from
+10 −235
Open

refactor: remove unreachable Partition delta-computation interface methods #1896

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
169 changes: 1 addition & 168 deletions src/graph/algorithms/leiden/partition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

import type { GraphAdapter } from './adapter.js';
import { accumulateInternalEdgeWeights, accumulateNodeAggregates } from './aggregate-helpers.js';
import { fget, fgetOrZero, iget, u8get } from './typed-array-helpers.js';
import { fget, iget, u8get } from './typed-array-helpers.js';

export interface CompactOptions {
keepOldOrder?: boolean;
Expand All @@ -29,14 +29,10 @@ export interface Partition {
resizeCommunities(newCount: number): void;
initializeAggregates(): void;
accumulateNeighborCommunityEdgeWeights(v: number): number;
getCandidateCommunityCount(): number;
getCandidateCommunityAt(i: number): number;
getNeighborEdgeWeightToCommunity(c: number): number;
getOutEdgeWeightToCommunity(c: number): number;
getInEdgeWeightFromCommunity(c: number): number;
deltaModularityUndirected(v: number, newC: number, gamma?: number): number;
deltaModularityDirected(v: number, newC: number, gamma?: number): number;
deltaCPM(v: number, newC: number, gamma?: number): number;
moveNodeToCommunity(v: number, newC: number): boolean;
compactCommunityIds(opts?: CompactOptions): void;
getCommunityMembers(): number[][];
Expand Down Expand Up @@ -229,163 +225,6 @@ function accumulateNeighborWeights(s: PartitionState, v: number): number {
return s.candidateCommunityCount;
}

/* ------------------------------------------------------------------ */
/* Extracted: modularity delta computations */
/* ------------------------------------------------------------------ */

function computeDeltaModularityUndirected(
s: PartitionState,
v: number,
newC: number,
gamma: number = 1.0,
): number {
const oldC: number = iget(s.nodeCommunity, v);
if (newC === oldC) return 0;
const twoM: number = s.graph.totalWeight;
const strengthV: number = fget(s.graph.strengthOut, v);
const weightToNew: number =
newC < s.neighborEdgeWeightToCommunity.length
? fget(s.neighborEdgeWeightToCommunity, newC) || 0
: 0;
const weightToOld: number = fget(s.neighborEdgeWeightToCommunity, oldC) || 0;
const totalStrengthNew: number =
newC < s.communityTotalStrength.length ? fget(s.communityTotalStrength, newC) : 0;
const totalStrengthOld: number = fget(s.communityTotalStrength, oldC);
const gain_remove: number = -(
weightToOld / twoM -
(gamma * (strengthV * totalStrengthOld)) / (twoM * twoM)
);
const gain_add: number =
weightToNew / twoM - (gamma * (strengthV * totalStrengthNew)) / (twoM * twoM);
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,
newC: number,
gamma: number = 1.0,
): number {
const oldC: number = iget(s.nodeCommunity, v);
if (newC === oldC) return 0;
const totalEdgeWeight: number = s.graph.totalWeight;
const strengthOutV: number = fget(s.graph.strengthOut, v);
const strengthInV: number = fget(s.graph.strengthIn, v);
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 =
(inFromNew + outToNew - inFromOld - outToOld + 2 * selfW) / totalEdgeWeight;
const deltaExpected: number =
(gamma *
(strengthOutV * (totalInStrengthNew - totalInStrengthOld) +
strengthInV * (totalOutStrengthNew - totalOutStrengthOld) +
2 * strengthOutV * strengthInV)) /
(totalEdgeWeight * totalEdgeWeight);
return deltaInternal - deltaExpected;
}

/** computeCpmEdgeWeights — directed branch: in+out weight, plus self-loop correction. */
function computeCpmEdgeWeightsDirected(
s: PartitionState,
v: number,
oldC: number,
newC: number,
): { wOld: number; wNew: number; selfCorrection: number } {
const wOld: number =
(fget(s.outEdgeWeightToCommunity, oldC) || 0) + (fget(s.inEdgeWeightFromCommunity, oldC) || 0);
const wNew: number =
newC < s.outEdgeWeightToCommunity.length
? (fget(s.outEdgeWeightToCommunity, newC) || 0) +
(fget(s.inEdgeWeightFromCommunity, newC) || 0)
: 0;
// Self-loop correction (see cpm.ts diffCPM)
const selfCorrection: number = 2 * (fget(s.graph.selfLoop, v) || 0);
return { wOld, wNew, selfCorrection };
}

/** computeCpmEdgeWeights — undirected branch: single neighbor-weight-to-community value. */
function computeCpmEdgeWeightsUndirected(
s: PartitionState,
oldC: number,
newC: number,
): { wOld: number; wNew: number; selfCorrection: number } {
const wOld: number = fget(s.neighborEdgeWeightToCommunity, oldC) || 0;
const wNew: number =
newC < s.neighborEdgeWeightToCommunity.length
? fget(s.neighborEdgeWeightToCommunity, newC) || 0
: 0;
return { wOld, wNew, selfCorrection: 0 };
}

/** Directed/undirected edge-weight-to-community split used by computeDeltaCPM. */
function computeCpmEdgeWeights(
s: PartitionState,
v: number,
oldC: number,
newC: number,
): { wOld: number; wNew: number; selfCorrection: number } {
return s.graph.directed
? computeCpmEdgeWeightsDirected(s, v, oldC, newC)
: computeCpmEdgeWeightsUndirected(s, oldC, newC);
}

function computeDeltaCPM(s: PartitionState, v: number, newC: number, gamma: number = 1.0): number {
const oldC: number = iget(s.nodeCommunity, v);
if (newC === oldC) return 0;
const { wOld: w_old, wNew: w_new, selfCorrection } = computeCpmEdgeWeights(s, v, oldC, newC);
const nodeSz: number = fget(s.graph.size, v) || 1;
const sizeOld: number = fget(s.communityTotalSize, oldC) || 0;
const sizeNew: number = newC < s.communityTotalSize.length ? fget(s.communityTotalSize, newC) : 0;
return w_new - w_old + selfCorrection - gamma * nodeSz * (sizeNew - sizeOld + nodeSz);
}

/* ------------------------------------------------------------------ */
/* Extracted: node move */
/* ------------------------------------------------------------------ */
Expand Down Expand Up @@ -594,17 +433,11 @@ export function makePartition(graph: GraphAdapter, options: MakePartitionOptions
},
initializeAggregates: () => initAggregates(s),
accumulateNeighborCommunityEdgeWeights: (v: number) => accumulateNeighborWeights(s, v),
getCandidateCommunityCount: (): number => s.candidateCommunityCount,
getCandidateCommunityAt: (i: number): number => iget(s.candidateCommunities, i),
getNeighborEdgeWeightToCommunity: (c: number): number =>
fget(s.neighborEdgeWeightToCommunity, c) || 0,
getOutEdgeWeightToCommunity: (c: number): number => fget(s.outEdgeWeightToCommunity, c) || 0,
getInEdgeWeightFromCommunity: (c: number): number => fget(s.inEdgeWeightFromCommunity, c) || 0,
deltaModularityUndirected: (v: number, newC: number, gamma?: number) =>
computeDeltaModularityUndirected(s, v, newC, gamma),
deltaModularityDirected: (v: number, newC: number, gamma?: number) =>
computeDeltaModularityDirected(s, v, newC, gamma),
deltaCPM: (v: number, newC: number, gamma?: number) => computeDeltaCPM(s, v, newC, gamma),
moveNodeToCommunity: (v: number, newC: number) => moveNode(s, v, newC),
compactCommunityIds: (opts?: CompactOptions) => compactIds(s, opts),
getCommunityMembers(): number[][] {
Expand Down
27 changes: 0 additions & 27 deletions src/graph/algorithms/leiden/typed-array-helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,33 +22,6 @@ 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;
Expand Down
49 changes: 9 additions & 40 deletions tests/graph/algorithms/leiden.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
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 ──────────────────────────────────────────────────────────
Expand Down Expand Up @@ -653,48 +652,18 @@ 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.
// "directed modularity" / "directed self-loops" fixtures above. These go
// through the live directed-modularity delta path — diffModularityDirected
// in modularity.ts, invoked via computeQualityGain (optimiser.ts) during the
// Leiden local-move and refinement phases — not Partition's deltaModularityDirected
// interface method, which issue #1770 found to be unreachable dead code (no
// caller ever invokes partition.deltaModularityDirected(...)) and removed.
// A future change that perturbs diffModularityDirected's formula or its
// bounds-checked reads 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', () => {
Expand Down
Loading