core: Graph cohesion + StakeCovariance windowed acceleration — 15th+16th consolidated (Amara corrections #3+#4)

[AceHack]

Consolidates DIRTY #329 (Amara correction #3) with new StakeCovariance module (Amara correction #4).

44 GraphTests passing. 4 of 8 Amara 17th-ferry corrections now shipped.

[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

Consolidates Graph cohesion metrics (internal density, exclusivity, conductance) and introduces a new StakeCovariance primitive for windowed stake-delta covariance and “acceleration”, with accompanying tests.

Changes:

• Added Graph.internalDensity, Graph.exclusivity, and Graph.conductance.
• Added StakeCovariance module with windowedDeltaCovariance, covarianceAcceleration, and aggregateAcceleration.
• Expanded Graph.Tests.fs with new unit tests covering these additions.

Reviewed changes

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

File	Description
src/Core/Graph.fs	Adds cohesion metrics and embeds new StakeCovariance APIs.
tests/Tests.FSharp/Algebra/Graph.Tests.fs	Adds unit tests for cohesion metrics and StakeCovariance functions.

[Copilot]

aggregateAcceleration hard-codes Map<int * int, double>, which makes the StakeCovariance API inconsistent with the rest of Core’s generic node/key handling (e.g., Graph<'N>). Consider making this generic over the key type (e.g., Map<'N * 'N, double> with 'N : comparison) or just accepting a sequence of acceleration values if the keys aren’t used.

Suggested change

	let aggregateAcceleration
	(pairAccelerations: Map<int * int, double>)
	let aggregateAcceleration<'N when 'N : comparison>
	(pairAccelerations: Map<'N * 'N, double>)

[Copilot]

Graph.fs now defines a second [<AutoOpen>] module (StakeCovariance). Most Core modules appear to be one concept per file (e.g., src/Core/RobustStats.fs, src/Core/TemporalCoordinationDetection.fs), and auto-opening additional APIs from Graph.fs can unexpectedly pollute Zeta.Core. Consider moving StakeCovariance into its own StakeCovariance.fs (and only marking it AutoOpen if you really want those identifiers imported everywhere).

Suggested change

[<AutoOpen>]

[Copilot]

In this test suite, open directives are consistently kept at the top of the file (e.g., Algebra/RobustStats.Tests.fs, Algebra/TemporalCoordinationDetection.Tests.fs). Adding open Zeta.Core.StakeCovariance mid-file is inconsistent; consider moving it to the top-level opens or qualifying the calls instead.

[Copilot]

P0: conductance treats a subset as “full” based on subset.Count = allNodes.Count, which can return None for non-full subsets that happen to have the same cardinality (e.g., {1;2;4} vs graph nodes {1;2;3}). Compare sets (or intersect with nodes g first) rather than comparing counts.

[Copilot]

windowedDeltaCovariance doc says it returns None for “zero variance / insufficient samples”, but the implementation always returns Some when n >= windowSize (including constant/zero-variance windows, where covariance is well-defined and should be 0.0). Either implement the documented degeneracy checks or update the comment to match the actual behavior (and clarify whether you intend population vs sample covariance, since you divide by windowSize).

[Copilot]

windowedDeltaCovariance uses n = min deltasA.Length deltasB.Length and then takes the trailing window ending at n. If the two series have different lengths, this silently drops the tail of the longer series and can misalign time indices. Consider requiring equal lengths (return None/error otherwise) or explicitly documenting the alignment/truncation semantics.

Suggested change

	/// trailing `windowSize` values. Returns None when either
	/// series has fewer than `windowSize` points or when a
	/// degenerate case (zero variance, insufficient samples)
	/// makes covariance undefined.
	let windowedDeltaCovariance
	(windowSize: int)
	(deltasA: double[])
	(deltasB: double[])
	: double option =
	let n = min deltasA.Length deltasB.Length
	if windowSize < 2 || n < windowSize then None
	else
	let start = n - windowSize
	let mutable meanA = 0.0
	let mutable meanB = 0.0
	for i in 0 .. windowSize - 1 do
	meanA <- meanA + deltasA.[start + i]
	meanB <- meanB + deltasB.[start + i]
	meanA <- meanA / double windowSize
	meanB <- meanB / double windowSize
	let mutable cov = 0.0
	for i in 0 .. windowSize - 1 do
	cov <- cov + (deltasA.[start + i] - meanA) *
	(deltasB.[start + i] - meanB)
	Some (cov / double windowSize)
	/// trailing `windowSize` values. Returns None when the
	/// series lengths differ, when either series has fewer
	/// than `windowSize` points, or when a degenerate case
	/// (zero variance, insufficient samples) makes covariance
	/// undefined. Equal lengths are required so the trailing
	/// window is aligned by time index in both series.
	let windowedDeltaCovariance
	(windowSize: int)
	(deltasA: double[])
	(deltasB: double[])
	: double option =
	if deltasA.Length <> deltasB.Length then None
	else
	let n = deltasA.Length
	if windowSize < 2 || n < windowSize then None
	else
	let start = n - windowSize
	let mutable meanA = 0.0
	let mutable meanB = 0.0
	for i in 0 .. windowSize - 1 do
	meanA <- meanA + deltasA.[start + i]
	meanB <- meanB + deltasB.[start + i]
	meanA <- meanA / double windowSize
	meanB <- meanB / double windowSize
	let mutable cov = 0.0
	for i in 0 .. windowSize - 1 do
	cov <- cov + (deltasA.[start + i] - meanA) *
	(deltasB.[start + i] - meanB)
	Some (cov / double windowSize)

[Copilot]

Pull request overview

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

[Copilot]

P2: The doc comment for windowedDeltaCovariance says it returns None for degenerate cases like zero variance, but the implementation always returns Some (often 0.0) whenever windowSize >= 2 and enough samples exist. Either implement the stated degeneracy checks or update the comment to reflect the actual behavior.

Suggested change

	/// trailing `windowSize` values. Returns None when either
	/// series has fewer than `windowSize` points or when a
	/// degenerate case (zero variance, insufficient samples)
	/// makes covariance undefined.
	/// trailing `windowSize` values. Returns None only when
	/// `windowSize < 2` or when either series has fewer than
	/// `windowSize` aligned points. Otherwise returns the
	/// trailing-window covariance, which may be `0.0` for
	/// constant windows or other zero-covariance inputs.

[Copilot]

P2: aggregateAcceleration materializes the map values into an array (Seq.toArray + Array.sum) just to compute an average. This adds an avoidable allocation; a Map.fold/Seq.fold that tracks (sum, count) would compute the mean in one pass without extra memory.

Suggested change

	let values = pairAccelerations |> Map.toSeq |> Seq.map snd |> Seq.toArray
	let sum = values |> Array.sum
	Some (sum / double values.Length)
	let sum, count =
	pairAccelerations
	|> Map.fold (fun (sum, count) _ acceleration -> sum + acceleration, count + 1) (0.0, 0)
	Some (sum / double count)

[Copilot]

P1: internalDensity counts self-loops in the numerator (because it only checks subset.Contains s && subset.Contains t) but the denominator uses |S|(|S|-1) which assumes ordered pairs with s <> t. Since this graph type explicitly allows self-loops, either exclude s = t edges from the numerator or change the denominator to include self-pairs (e.g., |S|^2) so the metric matches its definition.