[Bug] kmeansvals producer emits empty clusters #172

Bug描述 Describe the bug

Not a crash report — both panics are fixed, thank you (#142, and #164 which closed #157). This
is about the function that creates the condition, and about a test that now prevents fixing it.

utils.KMeansVals returns clusters with empty Items whenever it is asked for more
clusters than there are distinct values: several centers converge on the same value and the
losers win no members, but are still returned. Measured on a132a3b:

all identical, num=4                -> clusters=4, empty=3
two distinct values, num=5          -> clusters=5, empty=3
duplicates plus one outlier, num=6  -> clusters=6, empty=4

This is not an exotic input: a strategy with a fixed take-profit or stop closes many trades at
the same profit rate.

There are three call sites today, and all three are currently safe — but only two of them
because someone defended against this:

So the same defect has already had to be patched twice, in two files, five weeks apart, by two
different people — and the third site is safe by coincidence rather than by design.

Empty clusters do not only panic. They also give silently wrong answers. A caller that
never touches Items still gets a result structure that misdescribes itself:

input: 8 trades at 0.02, 2 trades at 0.05, num=5
len(res.Clusters) = 5      but only 2 clusters contain anything
=> mean group size computed as 10/5 = 2.0 instead of 10/2 = 5.0

No panic, no error — just a wrong number. This failure mode is not covered by either existing
guard, because both guards work by skipping empty clusters while iterating, not by correcting
the count.

Finally, the current behaviour is now locked in by a test. opt/reports_test.go:

clusters := utils.KMeansVals(rates, 4)
hasEmptyCluster := false
for _, cluster := range clusters.Clusters {
    hasEmptyCluster = hasEmptyCluster || len(cluster.Items) == 0
}
if !hasEmptyCluster {
    t.Fatal("expected duplicate profit rates to produce an empty KMeans cluster")
}

That is an accurate description of what happens today, but it means anyone who fixes
KMeansVals gets a red test and will reasonably conclude the behaviour is intentional. This is
probably the most important part of this report: the defect is not merely unfixed, it is
protected.

具体复现流程？Steps To Reproduce

Hermetic — no database, no exchange. Drop into utils/:

package utils

import (
	"slices"
	"testing"
)

func TestKMeansValsEmptyClusterContract(t *testing.T) {
	// 8 trades closed at one level, 2 at another — e.g. a fixed take-profit
	rates := []float64{0.02, 0.02, 0.02, 0.02, 0.02, 0.02, 0.02, 0.02, 0.05, 0.05}
	res := KMeansVals(rates, 5)

	empty, nonEmpty := 0, 0
	for _, c := range res.Clusters {
		if len(c.Items) == 0 {
			empty++
		} else {
			nonEmpty++
		}
	}
	t.Logf("clusters=%d empty=%d nonEmpty=%d", len(res.Clusters), empty, nonEmpty)

	// (a) the silently wrong answer, no panic involved
	t.Logf("mean group size: %.1f reported vs %.1f actual",
		float64(len(rates))/float64(len(res.Clusters)),
		float64(len(rates))/float64(nonEmpty))

	// (b) the panic, for any caller that reduces Items without checking
	defer func() { t.Logf("naive slices.Min over every cluster: %v", recover()) }()
	for _, c := range res.Clusters {
		_ = slices.Min(c.Items)
	}
}

Observed on main @ a132a3b:

clusters=5 empty=3 nonEmpty=2
mean group size: 2.0 reported vs 5.0 actual
naive slices.Min over every cluster: slices.Min: empty list

期望行为 Expected behavior

KMeansVals returns only clusters that contain at least one item, with RowGIds remapped to
match. Then len(res.Clusters) means what a caller would assume, no consumer needs a guard,
and the two existing guards become unreachable defensive code instead of load-bearing.

截图 Screenshots

None — the test output above is the whole signal.

运行环境 Running Environment

• OS: MacOS
• CPU: arm
• Version: banbox/banbot@main HEAD a132a3b (2026-07-28) / v0.4.2-beta.10

其他信息 Additional context

Suggested fix at the producer — drop empty clusters and remap the row group ids, immediately
before the return &ClusterRes{...} in KMeansVals:

remap := make([]int, len(resList))
kept := make([]*Cluster, 0, len(resList))
for i, c := range resList {
    if len(c.Items) == 0 {
        remap[i] = -1
        continue
    }
    remap[i] = len(kept)
    kept = append(kept, c)
}
if len(kept) < len(resList) {
    for i := range rowGids {
        rowGids[i] = remap[rowGids[i]]
    }
    resList = kept
}

remap[i] = -1 is unreachable by construction: a cluster is empty exactly when no row was
assigned to it, so no RowGIds entry can point at one.

Landing this requires one test change, and it is worth being explicit about it: the
precondition in TestGroupByProfitsHandlesEmptyKMeansClusters quoted above becomes false and
must be inverted (assert that no cluster is empty, and that every RowGIds[i] is in range).
The rest of that test passes unchanged — which is exactly the point: the consumer keeps
working, the condition it defends against simply stops occurring.

Disclosure, so this does not read as self-interest: we carried this producer patch in a fork
from mid-July and have now removed it, because PR #164 covers the only call paths our own
code reaches. We are proposing it for the codebase rather than for ourselves. Happy to open it
as a PR with the test change included.

The only visible behavioural consequence we measured is cosmetic, and arguably an improvement:
profit-range report buckets contain no empty groups. Order-level backtest output is unaffected
— we verified this both ways, with and without the patch, on several strategies.

Suggested regression test

TestKMeansValsNoEmptyClusters: feed several duplicate-heavy inputs across a range of num
and assert (1) no returned cluster has empty Items, and (2) every RowGIds[i] lies within
[0, len(Clusters)). The second assertion is the one that matters — it catches a remap that
drops clusters without renumbering the ids.

微信/QQ/Telegram/Discord/Email

• alx@nur.re