diff --git a/cmd/internal-tools/analyze/analysis.go b/cmd/internal-tools/analyze/analysis.go index dadda02..c6b9d5d 100644 --- a/cmd/internal-tools/analyze/analysis.go +++ b/cmd/internal-tools/analyze/analysis.go @@ -67,22 +67,28 @@ type analysis struct { Notes []string `json:"notes,omitempty"` } -const outcomeDescription = "steps to first violation, right-censored at the step budget" +const outcomeDescription = "steps to first violation, right-censored at the last step a clean run reached" -func analyse(arms []arm, now time.Time) analysis { - result, testable := baseAnalysis(arms, now) +func analyse(arms []arm, now time.Time) (analysis, error) { + result, testable, err := baseAnalysis(arms, now) + if err != nil { + return analysis{}, err + } if len(testable) >= 2 { result.Pairwise = comparePairs(testable) result.HolmFamilySize = countCorrected(result.Pairwise) } - return result + return result, nil } // analysePaired is the seed-matched design of the actuation ablation: two arms // running the same seeds, contrasted seed by seed rather than as two // independent samples. func analysePaired(arms []arm, now time.Time) (analysis, error) { - result, testable := baseAnalysis(arms, now) + result, testable, err := baseAnalysis(arms, now) + if err != nil { + return analysis{}, err + } if len(testable) != 2 { return analysis{}, fmt.Errorf("a paired comparison needs exactly two arms with usable runs, found %d", len(testable)) } @@ -102,7 +108,7 @@ func analysePaired(arms []arm, now time.Time) (analysis, error) { return result, nil } -func baseAnalysis(arms []arm, now time.Time) (analysis, []arm) { +func baseAnalysis(arms []arm, now time.Time) (analysis, []arm, error) { result := analysis{GeneratedAt: now, Outcome: outcomeDescription} for _, current := range arms { result.Arms = append(result.Arms, summarize(current)) @@ -118,6 +124,9 @@ func baseAnalysis(arms []arm, now time.Time) (analysis, []arm) { result.Notes = append(result.Notes, "arms with no usable runs are reported but left out of the log-rank test and the pairwise comparisons") } + if err := sameBudget(testable); err != nil { + return analysis{}, nil, err + } if len(testable) >= 2 { names := make([]string, len(testable)) groups := make([][]observation, len(testable)) @@ -128,7 +137,24 @@ func baseAnalysis(arms []arm, now time.Time) (analysis, []arm) { test := logRank(names, groups) result.LogRank = &test } - return result, testable + return result, testable, nil +} + +// sameBudget refuses arms that were given different exposure. A clean run is +// censored somewhere at or below its arm's budget, so the arm with the larger +// budget carries censored runs the smaller arm could not have produced, and +// every test that ranks the two against each other reads that as the arm +// surviving longer. It is the cross-arm form of what groupArms already refuses +// within one arm. +func sameBudget(arms []arm) error { + for index := 1; index < len(arms); index++ { + if arms[index].Budget != arms[0].Budget { + return fmt.Errorf("arm %q has step budget %d and arm %q has %d: "+ + "runs censored at different budgets cannot be compared", + arms[0].Name, arms[0].Budget, arms[index].Name, arms[index].Budget) + } + } + return nil } func countCorrected(pairs []pairwiseResult) int { diff --git a/cmd/internal-tools/analyze/analysis_test.go b/cmd/internal-tools/analyze/analysis_test.go index cddfd18..0b83203 100644 --- a/cmd/internal-tools/analyze/analysis_test.go +++ b/cmd/internal-tools/analyze/analysis_test.go @@ -1,8 +1,10 @@ package main import ( + "io" "math" "path/filepath" + "strings" "testing" "time" ) @@ -227,11 +229,14 @@ func TestAnalyse_ExcludedRunsNeverBecomeObservations(t *testing.T) { } func TestAnalyse_ArmWithNoUsableRunsIsReportedButNotTested(t *testing.T) { - result := analyse([]arm{ + result, err := analyse([]arm{ {Name: "a", Budget: 30, Runs: []classifiedRun{violatingRun(1, 4, 4), violatingRun(2, 6, 6)}}, {Name: "b", Budget: 30, Runs: []classifiedRun{cleanRun(1, 30), cleanRun(2, 30)}}, {Name: "c", Budget: 30, Runs: []classifiedRun{{Seed: 1, ExcludedBecause: reasonNonzeroExit}}}, }, time.Unix(0, 0).UTC()) + if err != nil { + t.Fatal(err) + } if len(result.Arms) != 3 { t.Fatalf("%d arms reported, want all 3", len(result.Arms)) @@ -250,9 +255,12 @@ func TestAnalyse_ArmWithNoUsableRunsIsReportedButNotTested(t *testing.T) { // With a single testable arm there is nothing to compare against, and the tool // must say so instead of producing a statistic. func TestAnalyse_SingleArmHasNoTests(t *testing.T) { - result := analyse([]arm{ + result, err := analyse([]arm{ {Name: "a", Budget: 30, Runs: []classifiedRun{violatingRun(1, 4, 4)}}, }, time.Unix(0, 0).UTC()) + if err != nil { + t.Fatal(err) + } if result.LogRank != nil || len(result.Pairwise) != 0 { t.Errorf("log-rank %+v pairwise %v, want neither", result.LogRank, result.Pairwise) } @@ -297,6 +305,53 @@ func TestComparePairs_A12DirectionFollowsStepCounts(t *testing.T) { } } +func writeCleanCampaign(t *testing.T, directory, name string, budget, steps, runs int) { + t.Helper() + seeds := make([]int, 0, runs) + records := make([]map[string]any, 0, runs) + for seed := 1; seed <= runs; seed++ { + seeds = append(seeds, seed) + records = append(records, map[string]any{ + "seed": seed, "exit_code": 0, "steps": steps, "actions": steps, "monotonic_millis": 60_000, + }) + } + writeCampaign(t, directory, map[string]any{"arm": name, "max_steps": budget, "seeds": seeds}, records) +} + +// Arms censored at different budgets are not on the same clock: every clean run +// of the wider arm outranks every clean run of the narrower one whatever the +// app did, so the rank-sum and the paired test reach a foregone conclusion the +// log-rank in the same report contradicts. groupArms already refuses this +// within one arm, and comparing across arms is the same hazard. +func TestRun_RefusesToCompareArmsCensoredAtDifferentBudgets(t *testing.T) { + cases := []struct { + name string + wideSteps int + arguments []string + }{ + {name: "identical runs under different budgets", wideSteps: 100}, + {name: "each arm run to its own budget", wideSteps: 400}, + {name: "paired", wideSteps: 400, arguments: []string{"--paired"}}, + } + for _, test := range cases { + root := t.TempDir() + wide := filepath.Join(root, "wide") + narrow := filepath.Join(root, "narrow") + writeCleanCampaign(t, wide, "wide", 400, test.wideSteps, 30) + writeCleanCampaign(t, narrow, "narrow", 100, 100, 30) + + err := run(append(test.arguments, wide, narrow), io.Discard, io.Discard) + if err == nil { + t.Fatalf("%s: arms censored at 400 and at 100 steps were compared without complaint", test.name) + } + for _, fragment := range []string{"wide", "400", "narrow", "100", "different budgets"} { + if !strings.Contains(err.Error(), fragment) { + t.Errorf("%s: error %q is missing %q", test.name, err, fragment) + } + } + } +} + func manyRuns(count, originStep int) []classifiedRun { runs := make([]classifiedRun, 0, count) for index := 0; index < count; index++ { diff --git a/cmd/internal-tools/analyze/end_to_end_test.go b/cmd/internal-tools/analyze/end_to_end_test.go index 7ce181e..effe935 100644 --- a/cmd/internal-tools/analyze/end_to_end_test.go +++ b/cmd/internal-tools/analyze/end_to_end_test.go @@ -90,7 +90,7 @@ func TestRun_EndToEndOverFixtureCampaignDirectories(t *testing.T) { text := stdout.String() for _, fragment := range []string{ - "steps to first violation, right-censored at the step budget", + "steps to first violation, right-censored at the last step a clean run reached", "log-rank across 2 arms", "pairwise wilcoxon rank-sum", "llm vs seeded", diff --git a/cmd/internal-tools/analyze/load.go b/cmd/internal-tools/analyze/load.go index 3210f09..87374fa 100644 --- a/cmd/internal-tools/analyze/load.go +++ b/cmd/internal-tools/analyze/load.go @@ -17,8 +17,8 @@ const ( ) // manifest mirrors the fields analyze reads from campaign.json. The step budget -// lives here rather than in any run, because it is what clean runs are censored -// at and every run in an arm has to share it. +// lives here rather than in any run, because it is the exposure every run in an +// arm was given and the ceiling a clean run can be censored at. type manifest struct { Arm string `json:"arm"` Generator string `json:"generator"` @@ -60,8 +60,8 @@ type runRecord struct { } // Exclusion reasons. A run that failed or timed out is missing data, not a -// censored observation: it never ran its budget, so treating it as a clean run -// that survived to the budget would bias the survival estimate downward. +// censored observation: it broke off, so its step count is not exposure the app +// survived and counting it as one would bias the survival estimate downward. const ( reasonLaunchError = "launch error" reasonTimedOut = "timed out" @@ -209,8 +209,8 @@ func classify(record runRecord, budget int) classifiedRun { // run ends, and timing that at the step that armed it would record a liveness // failure flushed at the budget as a violation found on the first step, which // is a number the run cannot support and which no censored run can be compared -// against: the budget is the clock the clean runs are censored on, so the events -// have to be on it too. A campaign written before the field existed carries no +// against: the end of the run is the clock the clean runs are censored on, so +// the events have to be on it too. A campaign written before the field existed carries no // detected step and keeps the origin. func eventStep(record runRecord, origin int) int { if record.FirstViolationDetectedStep == nil { @@ -271,7 +271,8 @@ func groupArms(directories []string) ([]arm, error) { } // observations returns the usable runs as survival data: an event at the step -// that armed the first violation, or a censored observation at the step budget. +// that armed the first violation, or a censored observation at the last step +// the run reached. func (a arm) observations() []observation { var result []observation for _, item := range a.Runs { @@ -287,13 +288,15 @@ func observationOf(item classifiedRun, budget int) observation { if item.Violated { return observation{Steps: float64(item.EventStep), Event: true} } - return observation{Steps: float64(budget), Event: false} + // A run that hit the campaign's wall clock stopped short of the budget, and + // the steps it never ran are not exposure it survived. + return observation{Steps: float64(min(item.Steps, budget)), Event: false} } // stepTimes is the observations flattened to plain numbers, censored runs held -// at the budget. Holding them there rather than dropping them is conservative: -// it can only understate how much sooner a violating arm finds its first -// defect, never overstate it. +// at the steps they ran. Holding them there rather than dropping them is +// conservative: it can only understate how much sooner a violating arm finds +// its first defect, never overstate it. func (a arm) stepTimes() []float64 { var result []float64 for _, item := range a.observations() { diff --git a/cmd/internal-tools/analyze/load_test.go b/cmd/internal-tools/analyze/load_test.go index c787bd7..f1ea053 100644 --- a/cmd/internal-tools/analyze/load_test.go +++ b/cmd/internal-tools/analyze/load_test.go @@ -31,18 +31,32 @@ func TestClassify_FailedAndTimedOutRunsAreMissingDataNotCensored(t *testing.T) { } } -func TestClassify_CleanRunIsCensoredAtTheBudget(t *testing.T) { - item := classify(runRecord{Seed: 4, Steps: 50, DurationMillis: 1000}, 50) - if item.ExcludedBecause != "" { - t.Fatalf("excluded because %q", item.ExcludedBecause) +// A run also ends when the campaign's wall clock does, so a clean run can stop +// well short of the budget. Censoring it at the budget would credit it with +// steps it never ran, and a slower arm loses fewer steps in the same wall clock +// than a fast one, so the credit does not cancel between arms. +func TestClassify_CleanRunIsCensoredAtTheStepsItRan(t *testing.T) { + cases := []struct { + name string + steps int + budget int + censored float64 + }{ + {"stopped by the wall clock short of the budget", 12, 400, 12}, + {"ran the whole budget", 400, 400, 400}, + {"recorded more steps than the manifest budget", 420, 400, 400}, } - if item.Violated { - t.Error("clean run marked as violated") - } - current := arm{Budget: 50, Runs: []classifiedRun{item}} - observations := current.observations() - if len(observations) != 1 || observations[0].Event || observations[0].Steps != 50 { - t.Errorf("observations %+v, want one censored observation at 50", observations) + for _, test := range cases { + item := classify(runRecord{Seed: 4, Steps: test.steps, DurationMillis: 1000}, test.budget) + if item.ExcludedBecause != "" || item.Violated { + t.Fatalf("%s: run %+v, want a usable clean run", test.name, item) + } + current := arm{Budget: test.budget, Runs: []classifiedRun{item}} + observations := current.observations() + if len(observations) != 1 || observations[0].Event || observations[0].Steps != test.censored { + t.Errorf("%s: observations %+v, want one censored observation at %v", + test.name, observations, test.censored) + } } } diff --git a/cmd/internal-tools/analyze/main.go b/cmd/internal-tools/analyze/main.go index 47e8d2d..be84c6e 100644 --- a/cmd/internal-tools/analyze/main.go +++ b/cmd/internal-tools/analyze/main.go @@ -1,6 +1,6 @@ // Command analyze reduces campaign directories to the statistics the // evaluation reports. The primary outcome is steps to first violation, with -// clean runs right-censored at the step budget rather than discarded: defect +// clean runs right-censored where they stopped rather than discarded: defect // yield per run is a binary that would need on the order of eighty runs an arm // to separate, while survival analysis uses every run, including the clean ones. package main @@ -24,7 +24,7 @@ Usage: Each directory is one produced by the campaign tool and must hold campaign.json and runs.jsonl. Directories sharing an arm label are pooled and must agree on -the step budget. +the step budget, and arms compared against each other must agree on it too. One invocation is one research question: Holm corrects across the comparisons it produces and across nothing else. @@ -87,7 +87,10 @@ func run(arguments []string, stdout, stderr io.Writer) error { return err } } else { - result = analyse(arms, time.Now().UTC()) + result, err = analyse(arms, time.Now().UTC()) + if err != nil { + return err + } } result.Question = question writeReport(result, stdout) diff --git a/cmd/internal-tools/analyze/paired.go b/cmd/internal-tools/analyze/paired.go index 792ed20..16fbd57 100644 --- a/cmd/internal-tools/analyze/paired.go +++ b/cmd/internal-tools/analyze/paired.go @@ -174,7 +174,7 @@ type pairedComparison struct { } // pairArms matches the two arms by seed and contrasts them pair by pair. -// Censored runs enter at the step budget, the same convention the unpaired +// Censored runs enter at the steps they ran, the same convention the unpaired // comparison uses. A seed usable in one arm and not the other is named rather // than dropped silently, because that is a host that lost a run and it is what // the campaign manifest exists to make visible. diff --git a/cmd/internal-tools/analyze/ranksum_test.go b/cmd/internal-tools/analyze/ranksum_test.go index 4a61dbc..b63b895 100644 --- a/cmd/internal-tools/analyze/ranksum_test.go +++ b/cmd/internal-tools/analyze/ranksum_test.go @@ -58,7 +58,7 @@ func TestVargaDelaneyA12_BoundaryCases(t *testing.T) { // The counting definition and the rank-sum route must agree, including when the // samples are tied against each other, which is the case the evaluation data is -// always in because censored runs are all held at the budget. +// usually in because censored runs pile up on the step they stopped at. func TestVargaDelaneyA12_AgreesWithRankSumStatistic(t *testing.T) { cases := [][2][]float64{ {{1, 2, 3}, {2, 3, 4}}, diff --git a/cmd/internal-tools/analyze/report.go b/cmd/internal-tools/analyze/report.go index f9f8d75..57e9bdb 100644 --- a/cmd/internal-tools/analyze/report.go +++ b/cmd/internal-tools/analyze/report.go @@ -99,7 +99,7 @@ func writeReport(result analysis, out io.Writer) { } if len(result.Pairwise) > 0 { - fmt.Fprintln(out, "\npairwise wilcoxon rank-sum, censored runs held at the budget") + fmt.Fprintln(out, "\npairwise wilcoxon rank-sum, censored runs held at the steps they ran") fmt.Fprintln(out, "a12 above 0.5 means the first arm takes more steps to its first violation") writeTable(out, []string{"comparison", "n1", "n2", "u", "a12", "p", "holm p"}, func(add func(...string)) { for _, pair := range result.Pairwise { @@ -130,7 +130,7 @@ func writeReport(result analysis, out io.Writer) { } func writePaired(out io.Writer, comparison pairedComparison) { - fmt.Fprintf(out, "\npaired per-seed difference, %s minus %s, censored runs held at the budget\n", + fmt.Fprintf(out, "\npaired per-seed difference, %s minus %s, censored runs held at the steps they ran\n", comparison.First, comparison.Second) fmt.Fprintf(out, "%d seed pair(s): %s sooner in %d, %s sooner in %d, tied in %d\n", comparison.Pairs, comparison.First, comparison.FirstSooner, diff --git a/cmd/internal-tools/analyze/survival.go b/cmd/internal-tools/analyze/survival.go index e8a4ff2..28ac2b0 100644 --- a/cmd/internal-tools/analyze/survival.go +++ b/cmd/internal-tools/analyze/survival.go @@ -7,8 +7,7 @@ import ( // observation is one run reduced to what the survival analysis needs: the step // count at which it left the risk set, and whether it left because a violation -// was found (an event) or because it exhausted the step budget without one -// (right-censored). A run that failed or timed out is neither and never +// was found (an event) or because the run ended without one (right-censored). A run that failed or timed out is neither and never // reaches this type. type observation struct { Steps float64