core: Graph.modularityScore — 11th graduation (Newman modularity)

[AceHack]

Second cartel-detection primitive per Graph ADR #316. Newman modularity Q for caller-supplied node partition.

Discovered theoretical calibration reality mid-tick: when cartel clique dominates total edge weight, Q is inherently compressed even for perfect partition. Test comment documents this; future toy detector calibrates vs null-baseline.

22 tests passing. Next: toy cartel detector property test combining eigenvalue + modularity.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[Copilot]

Pull request overview

Adds Newman modularity (Q) as a second cartel-detection primitive on the ZSet-backed Graph<'N> substrate, plus unit tests to validate basic modularity behavior on small synthetic graphs and a cartel-injection scenario.

Changes:

• Implement Graph.modularityScore to compute Newman modularity Q for a caller-supplied node partition.
• Add test coverage for empty graphs, single-community partitions, well-separated communities, and a cartel clique injection scenario.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
src/Core/Graph.fs	Adds Graph.modularityScore implementation and extensive doc comment describing the modularity primitive.
tests/Tests.FSharp/Algebra/Graph.Tests.fs	Adds unit tests exercising Graph.modularityScore across several graph/partition scenarios.

[Copilot]

twoM is used as the "2m" normalization constant. Because Graph supports signed weights (retractions), it’s possible for twoM to be negative; the current guard only checks twoM = 0.0, which can yield a negative normalization and surprising Q values. Consider defining modularity only for nonnegative total weight (e.g., return None when twoM <= 0.0 and/or when any symmetrized edge weight is negative), or document the intended signed-weight semantics explicitly.

Suggested change

	if twoM = 0.0 then None
	if twoM <= 0.0 then None

[Copilot]

The implementation builds dense n×n adjacency/symmetry matrices and then performs nested i/j loops, making runtime and memory O(n²) regardless of edge count. Since Graph is ZSet-backed and likely sparse, consider a sparse computation (iterate only existing edges and use per-community degree/weight aggregates) to keep this usable for larger graphs.

Suggested change

	let idx =
	nodeList
	|> List.mapi (fun i node -> node, i)
	|> Map.ofList
	// Symmetrized adjacency A_sym[i,j] = (A[i,j] + A[j,i]) / 2
	let adj = Array2D.create n n 0.0
	let span = g.Edges.AsSpan()
	for k in 0 .. span.Length - 1 do
	let entry = span.[k]
	let (s, t) = entry.Key
	let i = idx.[s]
	let j = idx.[t]
	adj.[i, j] <- adj.[i, j] + double entry.Weight
	let sym = Array2D.create n n 0.0
	for i in 0 .. n - 1 do
	for j in 0 .. n - 1 do
	sym.[i, j] <- (adj.[i, j] + adj.[j, i]) / 2.0
	// Weighted degree k_i = sum_j A_sym[i, j]
	let k = Array.create n 0.0
	for i in 0 .. n - 1 do
	let mutable acc = 0.0
	for j in 0 .. n - 1 do
	acc <- acc + sym.[i, j]
	k.[i] <- acc
	// 2m = sum of all degrees (undirected)
	let twoM =
	let mutable acc = 0.0
	for i in 0 .. n - 1 do
	acc <- acc + k.[i]
	acc
	if twoM = 0.0 then None
	else
	// Community label per node: partition lookup, or
	// node-index-based-singleton when missing
	let community i =
	let node = nodeList.[i]
	match Map.tryFind node partition with
	| Some c -> c
	| None -> -(i + 1) // unique negative = singleton
	let mutable q = 0.0
	for i in 0 .. n - 1 do
	for j in 0 .. n - 1 do
	if community i = community j then
	let expected = (k.[i] * k.[j]) / twoM
	q <- q + (sym.[i, j] - expected)
	Some (q / twoM)
	let communityByNode =
	nodeList
	|> List.mapi (fun i node ->
	let community =
	match Map.tryFind node partition with
	| Some c -> c
	| None -> -(i + 1) // unique negative = singleton
	node, community)
	|> Map.ofList
	let addToDictionary
	(dict: System.Collections.Generic.Dictionary<'K, double>)
	key
	delta =
	match dict.TryGetValue key with
	| true, value -> dict.[key] <- value + delta
	| false, _ -> dict.[key] <- delta
	let degreeByNode =
	System.Collections.Generic.Dictionary<'N, double>()
	let internalWeightByCommunity =
	System.Collections.Generic.Dictionary<int, double>()
	let degreeByCommunity =
	System.Collections.Generic.Dictionary<int, double>()
	let span = g.Edges.AsSpan()
	for k in 0 .. span.Length - 1 do
	let entry = span.[k]
	let (s, t) = entry.Key
	let w = double entry.Weight
	// Weighted degree k_i = sum_j A_sym[i, j].
	// Under A_sym = (A + A^T) / 2, each directed edge
	// contributes w/2 to its source degree and w/2 to
	// its target degree. Self-loops still contribute w.
	addToDictionary degreeByNode s (w / 2.0)
	addToDictionary degreeByNode t (w / 2.0)
	let sCommunity = communityByNode.[s]
	let tCommunity = communityByNode.[t]
	// Sum of A_sym[i, j] over node pairs within a
	// community equals the total directed edge weight
	// whose endpoints both lie in that community.
	if sCommunity = tCommunity then
	addToDictionary internalWeightByCommunity sCommunity w
	// 2m = sum of all weighted degrees in the symmetrized
	// undirected view.
	let twoM =
	let mutable acc = 0.0
	for kvp in degreeByNode do
	acc <- acc + kvp.Value
	acc
	if twoM = 0.0 then None
	else
	for kvp in degreeByNode do
	let community = communityByNode.[kvp.Key]
	addToDictionary degreeByCommunity community kvp.Value
	let mutable q = 0.0
	for kvp in degreeByCommunity do
	let community = kvp.Key
	let communityDegree = kvp.Value
	let internalWeight =
	match internalWeightByCommunity.TryGetValue community with
	| true, value -> value
	| false, _ -> 0.0
	let degreeFraction = communityDegree / twoM
	q <- q + (internalWeight / twoM) - (degreeFraction * degreeFraction)
	Some q

[Copilot]

community i does a Map.tryFind for every (i,j) pair inside the O(n²) loop. Even for moderate n, this adds a lot of repeated work (and allocations/lookup overhead). Precompute an int[]/array of community labels for all nodes once before the nested loop, then compare array entries inside the loop.

Suggested change

	let community i =
	let node = nodeList.[i]
	match Map.tryFind node partition with
	| Some c -> c
	| None -> -(i + 1) // unique negative = singleton
	let mutable q = 0.0
	for i in 0 .. n - 1 do
	for j in 0 .. n - 1 do
	if community i = community j then
	let communities =
	Array.init n (fun i ->
	let node = nodeList.[i]
	match Map.tryFind node partition with
	| Some c -> c
	| None -> -(i + 1)) // unique negative = singleton
	let mutable q = 0.0
	for i in 0 .. n - 1 do
	for j in 0 .. n - 1 do
	if communities.[i] = communities.[j] then

[Copilot]

Docstring says the function returns None when "every node is unassigned", but the implementation treats missing nodes as singleton communities and will still return Some for any graph with nonzero total weight (even if partition is empty). Please either update the doc comment to match the implemented behavior, or change the behavior to actually return None when partition assigns no nodes (and document that choice).

Suggested change

	/// when the graph is empty or every node is unassigned.
	/// Nodes missing from `partition` are treated as singleton
	/// groups (each in a unique trivial community).
	/// when the graph is empty or the symmetrized graph has
	/// zero total edge weight. Nodes missing from `partition`
	/// are treated as singleton groups (each in a unique
	/// trivial community), including the case where
	/// `partition` is empty.

[Copilot]

This doc comment introduces contributor/agent name attribution ("Implementation Otto …" / "Provenance …"). Repo convention is to avoid names in code/docs and use role references instead (docs/AGENT-BEST-PRACTICES.md:284-290). Please rewrite these lines to remove names and keep the provenance info in a role- or artifact-based form (e.g., ADR/doc IDs only).

Suggested change

	/// > 0.1 or Q > 0.4". Implementation Otto (11th graduation).
	/// > 0.1 or Q > 0.4". Implementation tracked under the 11th
	/// graduation artifacts.

[AceHack]

Superseded by consolidated PR. Content re-applied there.