Skip to content

refactor: reduce cyclomatic complexity of computeDeltaModularityDirected#1872

Closed
carlos-alm wants to merge 1 commit into
fix/issue-1754-domain-search-console-logfrom
fix/issue-1755-leiden-deltamod-directed-complexity
Closed

refactor: reduce cyclomatic complexity of computeDeltaModularityDirected#1872
carlos-alm wants to merge 1 commit into
fix/issue-1754-domain-search-console-logfrom
fix/issue-1755-leiden-deltamod-directed-complexity

Conversation

@carlos-alm

Copy link
Copy Markdown
Contributor

Summary

  • computeDeltaModularityDirected (cyc=11, threshold 10) had a documented ||0 asymmetry across its 4 bounds-checked getter call sites, flagged as needing investigation before any unification (risk of masking a real NaN case).
  • Confirmed the asymmetry is behaviorally inert: all arrays involved are dense, zero-initialized Float64Arrays, and every value entering them is pre-scrubbed through +x || 0 at ingestion (adapter.ts) — NaN/undefined can never reach these arrays. Cross-checked 3 ways: the same inconsistency exists within computeDeltaCPM on the same array, modularity.ts's independently-written diffModularityDirected reproduces the identical split, and the asymmetry already existed unchanged in the original vendored ngraph.leiden source at the initial vendoring commit.
  • Added a shared fgetOrZero helper (documented with this finding), applied uniformly to all 8 reads, and extracted computeDirectedEdgeWeightTerms/computeDirectedStrengthTerms. Cyclomatic 11→3, cognitive 10→2.

Closes #1755

Test plan

  • npm test — full suite green (3519 passed, 0 failed)
  • npm run lint — clean
  • Exact (not approximate) behavior-preservation check: 12 combinations (file/function-level × 3 seeds × 2 resolutions) on this repo's own 701-file/8833-function dependency graph, pre-fix vs. post-fix — byte-for-byte identical quality scores and community assignments, after controlling for an unrelated native-vs-JS-engine resolution difference in the test environment (not a regression)
  • New tests pin fgetOrZero's bounds/zero-fill contract and exact-value regression tests for computeDeltaModularityDirected's output

Filed #1871 for an out-of-scope finding: makePartition (cyc=10, pre-existing, unrelated) also exceeds the threshold.

Stacked on #1870 (base branch fix/issue-1754-domain-search-console-log) — only the leiden/test diff is this issue's change.

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
@greptile-apps

greptile-apps Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR reduces the cyclomatic complexity of computeDeltaModularityDirected (from 11 to 3) by extracting a shared fgetOrZero helper and two private term-computation functions, after thoroughly verifying that the pre-existing || 0 asymmetry across the four read sites was behaviorally inert.

  • typed-array-helpers.ts: New fgetOrZero function encapsulates the bounds-check + NaN-guard pattern previously hand-rolled at each call site, with a detailed doc comment explaining both why the guard is safe today and why it's kept for future-proofing.
  • partition.ts: computeDirectedEdgeWeightTerms and computeDirectedStrengthTerms replace the four inline ternary reads in computeDeltaModularityDirected; all other callers in the file are untouched.
  • leiden.test.ts: Four new unit tests pin fgetOrZero's contract, and two exact floating-point regression tests pin the directed modularity output for both existing directed fixtures.

Confidence Score: 5/5

Clean refactor with no behavioral changes; the extracted helpers and shared accessor directly replace the original hand-rolled ternaries, and exact floating-point regression tests confirm identical output.

The change is purely structural: the formula in computeDeltaModularityDirected is unchanged, the || 0 asymmetry was carefully investigated and confirmed inert, and three independent cross-checks support the author's analysis. New tests pin both the helper contract and exact output values.

No files require special attention.

Important Files Changed

Filename Overview
src/graph/algorithms/leiden/typed-array-helpers.ts Adds fgetOrZero: a well-documented bounds-checked accessor that consolidates the scattered pattern; thorough doc comment explains why
src/graph/algorithms/leiden/partition.ts Extracts computeDirectedEdgeWeightTerms and computeDirectedStrengthTerms helpers to replace the inline bounds-check ternaries in computeDeltaModularityDirected; reduces cyclomatic complexity from 11 to 3 with no behavioral change.
tests/graph/algorithms/leiden.test.ts Adds unit tests for fgetOrZero covering in-bounds, out-of-bounds, zero-entry, and NaN cases, plus exact floating-point regression tests for computeDeltaModularityDirected output on two graph fixtures.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[computeDeltaModularityDirected] --> B[computeDirectedEdgeWeightTerms]
    A --> C[computeDirectedStrengthTerms]
    B --> D[fgetOrZero inEdgeWeightFromCommunity newC]
    B --> E[fgetOrZero outEdgeWeightToCommunity newC]
    B --> F[fgetOrZero inEdgeWeightFromCommunity oldC]
    B --> G[fgetOrZero outEdgeWeightToCommunity oldC]
    C --> H[fgetOrZero communityTotalInStrength newC]
    C --> I[fgetOrZero communityTotalOutStrength newC]
    C --> J[fgetOrZero communityTotalInStrength oldC]
    C --> K[fgetOrZero communityTotalOutStrength oldC]
    D & E & F & G --> L[deltaInternal]
    H & I & J & K --> M[deltaExpected]
    L & M --> N[return deltaInternal - deltaExpected]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[computeDeltaModularityDirected] --> B[computeDirectedEdgeWeightTerms]
    A --> C[computeDirectedStrengthTerms]
    B --> D[fgetOrZero inEdgeWeightFromCommunity newC]
    B --> E[fgetOrZero outEdgeWeightToCommunity newC]
    B --> F[fgetOrZero inEdgeWeightFromCommunity oldC]
    B --> G[fgetOrZero outEdgeWeightToCommunity oldC]
    C --> H[fgetOrZero communityTotalInStrength newC]
    C --> I[fgetOrZero communityTotalOutStrength newC]
    C --> J[fgetOrZero communityTotalInStrength oldC]
    C --> K[fgetOrZero communityTotalOutStrength oldC]
    D & E & F & G --> L[deltaInternal]
    H & I & J & K --> M[deltaExpected]
    L & M --> N[return deltaInternal - deltaExpected]
Loading

Reviews (1): Last reviewed commit: "refactor: reduce cyclomatic complexity o..." | Re-trigger Greptile

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Codegraph Impact Analysis

4 functions changed7 callers affected across 2 files

  • computeDirectedEdgeWeightTerms in src/graph/algorithms/leiden/partition.ts:268 (4 transitive callers)
  • computeDirectedStrengthTerms in src/graph/algorithms/leiden/partition.ts:285 (4 transitive callers)
  • computeDeltaModularityDirected in src/graph/algorithms/leiden/partition.ts:303 (4 transitive callers)
  • fgetOrZero in src/graph/algorithms/leiden/typed-array-helpers.ts:48 (4 transitive callers)

@carlos-alm carlos-alm deleted the branch fix/issue-1754-domain-search-console-log July 6, 2026 21:28
@carlos-alm carlos-alm closed this Jul 6, 2026
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 6, 2026
@carlos-alm

Copy link
Copy Markdown
Contributor Author

Recreated as #1925 — this PR was inadvertently closed when its stacked base branch was deleted after #1870 merged. Continuing there.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant