From 4bcd82a8a6feaa30994af2304e324ea550006aba Mon Sep 17 00:00:00 2001 From: PJ Date: Tue, 18 Aug 2026 20:24:10 +0530 Subject: [PATCH] fix(analyze): write an undefined paired p-value as null, not as NaN A paired contrast where censoring orders no pair has no p-value, and JSON has no NaN, so --json failed with 'marshal summary: json: unsupported value: NaN' and wrote no summary at all after printing a complete report. The two fields join the medians and the rates already carried as pointers, undefined reading as null in the summary and n/a in the report. Reachable since a clean run started being censored where it stopped: an arm the wall clock stops before its partner ever violates orders nothing. --- cmd/internal-tools/analyze/analysis.go | 5 ++-- cmd/internal-tools/analyze/censoring_test.go | 30 ++++++++++++++++++++ cmd/internal-tools/analyze/paired.go | 21 +++++++------- cmd/internal-tools/analyze/paired_test.go | 10 +++---- cmd/internal-tools/analyze/planted_test.go | 8 +++--- cmd/internal-tools/analyze/report.go | 9 +++++- 6 files changed, 60 insertions(+), 23 deletions(-) diff --git a/cmd/internal-tools/analyze/analysis.go b/cmd/internal-tools/analyze/analysis.go index c836a5b..2ff4f68 100644 --- a/cmd/internal-tools/analyze/analysis.go +++ b/cmd/internal-tools/analyze/analysis.go @@ -105,8 +105,9 @@ func analysePaired(arms []arm, now time.Time) (analysis, error) { return analysis{}, fmt.Errorf("arms %q and %q share no seed with a usable run in both", testable[0].Name, testable[1].Name) } - if !math.IsNaN(comparison.PValue) { - comparison.HolmPValue = holm([]float64{comparison.PValue})[0] + if comparison.PValue != nil { + adjusted := holm([]float64{*comparison.PValue})[0] + comparison.HolmPValue = &adjusted result.HolmFamilySize = 1 } result.Paired = &comparison diff --git a/cmd/internal-tools/analyze/censoring_test.go b/cmd/internal-tools/analyze/censoring_test.go index 6a9f2ec..7574cc1 100644 --- a/cmd/internal-tools/analyze/censoring_test.go +++ b/cmd/internal-tools/analyze/censoring_test.go @@ -1,8 +1,11 @@ package main import ( + "bytes" + "io" "math" "path/filepath" + "strings" "testing" ) @@ -116,3 +119,30 @@ func TestRun_PairedContrastFollowsWhatCensoringDetermines(t *testing.T) { t.Errorf("a12 within pairs %.4f, want above 0.5", paired.A12) } } + +// Nothing orders any pair here, so there is no test to report. The summary has +// to say that rather than failing to write a number that does not exist: a +// NaN p-value is not JSON and the whole summary went unwritten behind it. +func TestRun_PairedContrastWithNoOrderedPairSaysSo(t *testing.T) { + earlyDirectory, lateDirectory := wallClockArms(t, stoppedShort(20, 12), violatedAt(20, 100)) + result := analyseCampaigns(t, "--paired", earlyDirectory, lateDirectory) + + paired := *result.Paired + if paired.Pairs != 20 || paired.Unordered != 20 { + t.Fatalf("paired %+v, want twenty pairs and all of them unordered", paired) + } + if paired.PValue != nil || paired.HolmPValue != nil { + t.Errorf("p %v and holm p %v, want both undefined", paired.PValue, paired.HolmPValue) + } + if result.HolmFamilySize != 0 { + t.Errorf("holm family of %d, want none where nothing was tested", result.HolmFamilySize) + } + + var stdout bytes.Buffer + if err := run([]string{"--paired", earlyDirectory, lateDirectory}, &stdout, io.Discard); err != nil { + t.Fatal(err) + } + if !strings.Contains(stdout.String(), "sign test over the 0 ordered pair(s), p n/a, holm p n/a") { + t.Errorf("report does not say the test was not run:\n%s", stdout.String()) + } +} diff --git a/cmd/internal-tools/analyze/paired.go b/cmd/internal-tools/analyze/paired.go index 7b75e36..90c74df 100644 --- a/cmd/internal-tools/analyze/paired.go +++ b/cmd/internal-tools/analyze/paired.go @@ -65,9 +65,12 @@ type pairedComparison struct { // of matched seeds on which the first arm took more steps, an unordered pair // counting as half. A matched design has no reason to compare the two arms // as pooled bags of runs when each seed has a partner. - A12 float64 `json:"a12_within_pairs"` - PValue float64 `json:"p_value"` - HolmPValue float64 `json:"holm_p_value"` + A12 float64 `json:"a12_within_pairs"` + // PValue is undefined, and null in the summary, when censoring left no pair + // ordered: there is nothing for the test to be a test of, and JSON has no + // way to write the number that is not there. + PValue *float64 `json:"p_value"` + HolmPValue *float64 `json:"holm_p_value"` } // pairArms matches the two arms by seed and contrasts them pair by pair. A seed @@ -84,13 +87,7 @@ func pairArms(first, second arm) (pairedComparison, error) { return pairedComparison{}, err } - comparison := pairedComparison{ - First: first.Name, - Second: second.Name, - A12: math.NaN(), - PValue: math.NaN(), - HolmPValue: math.NaN(), - } + comparison := pairedComparison{First: first.Name, Second: second.Name, A12: math.NaN()} var differences []float64 for _, seed := range sortedSeeds(firstBySeed, secondBySeed) { left, inFirst := firstBySeed[seed] @@ -130,7 +127,9 @@ func pairArms(first, second arm) (pairedComparison, error) { comparison.Sign = -1 } comparison.A12 = (float64(comparison.SecondSooner) + 0.5*float64(comparison.Unordered)) / float64(comparison.Pairs) - comparison.PValue = signTest(comparison.FirstSooner, comparison.SecondSooner) + if tested := signTest(comparison.FirstSooner, comparison.SecondSooner); !math.IsNaN(tested) { + comparison.PValue = &tested + } return comparison, nil } diff --git a/cmd/internal-tools/analyze/paired_test.go b/cmd/internal-tools/analyze/paired_test.go index b6950df..2195ca8 100644 --- a/cmd/internal-tools/analyze/paired_test.go +++ b/cmd/internal-tools/analyze/paired_test.go @@ -87,7 +87,7 @@ func TestPairArms_ScoresEachPairByWhichRunOutlivedTheOther(t *testing.T) { t.Errorf("median difference %v over %d pair(s), want 10 over 2", comparison.MedianDifference, comparison.BothViolated) } - if want := signTest(0, 2); comparison.PValue != want { + if want := signTest(0, 2); comparison.PValue == nil || *comparison.PValue != want { t.Errorf("p %v, want the sign test's %v over the two ordered pairs", comparison.PValue, want) } } @@ -105,8 +105,8 @@ func TestPairArms_PairsOfCleanRunsAreNotEvidence(t *testing.T) { if comparison.Unordered != 2 || comparison.Sign != 0 { t.Errorf("comparison %+v, want both pairs unordered and no direction", comparison) } - if !math.IsNaN(comparison.PValue) { - t.Errorf("p %v, want undefined with no ordered pair", comparison.PValue) + if comparison.PValue != nil { + t.Errorf("p %v, want undefined with no ordered pair", *comparison.PValue) } if comparison.MedianDifference != nil { t.Errorf("median difference %v, want undefined where no pair has two violations", @@ -181,8 +181,8 @@ func TestPairArms_DirectionReversesWithTheArms(t *testing.T) { t.Errorf("median differences %v and %v, want opposites", *forward.MedianDifference, *reversed.MedianDifference) } - if math.Abs(forward.PValue-reversed.PValue) > 1e-12 { - t.Errorf("p-values %v and %v, want the same two-sided value", forward.PValue, reversed.PValue) + if math.Abs(*forward.PValue-*reversed.PValue) > 1e-12 { + t.Errorf("p-values %v and %v, want the same two-sided value", *forward.PValue, *reversed.PValue) } if math.Abs(forward.A12+reversed.A12-1) > 1e-12 { t.Errorf("a12 %v and %v, want them to sum to 1", forward.A12, reversed.A12) diff --git a/cmd/internal-tools/analyze/planted_test.go b/cmd/internal-tools/analyze/planted_test.go index 3882fe8..880a3ea 100644 --- a/cmd/internal-tools/analyze/planted_test.go +++ b/cmd/internal-tools/analyze/planted_test.go @@ -647,11 +647,11 @@ func TestPlanted_PairedComparisonRecoversTheShiftAndItsSign(t *testing.T) { if want := 0.5 * float64(unordered) / 30; paired.A12 != want { t.Errorf("a12 within pairs %v, want %v where no pair favours the first arm", paired.A12, want) } - if paired.PValue > 0.001 { + if paired.PValue == nil || *paired.PValue > 0.001 { t.Errorf("p-value %v for a shift planted in every pair", paired.PValue) } - if paired.HolmPValue != paired.PValue { - t.Errorf("holm p %v in a family of one, want the raw %v", paired.HolmPValue, paired.PValue) + if *paired.HolmPValue != *paired.PValue { + t.Errorf("holm p %v in a family of one, want the raw %v", *paired.HolmPValue, *paired.PValue) } } @@ -665,7 +665,7 @@ func TestPlanted_PairedNullIsNotCalledSignificantAboveItsLevel(t *testing.T) { for replicate := 0; replicate < replicates; replicate++ { first, second := plantTwoArms(t, int64(5000+replicate), 30, model, model) result := analyseCampaigns(t, "--paired", first, second) - if result.Paired.PValue < 0.05 { + if *result.Paired.PValue < 0.05 { rejected++ } } diff --git a/cmd/internal-tools/analyze/report.go b/cmd/internal-tools/analyze/report.go index d915791..8dd99f3 100644 --- a/cmd/internal-tools/analyze/report.go +++ b/cmd/internal-tools/analyze/report.go @@ -149,7 +149,7 @@ func writePaired(out io.Writer, comparison pairedComparison) { formatStepDifference(comparison.MedianDifference), comparison.BothViolated, comparison.Sign, comparison.A12) fmt.Fprintf(out, "sign test over the %d ordered pair(s), p %s, holm p %s\n", comparison.FirstSooner+comparison.SecondSooner, - formatPValue(comparison.PValue), formatPValue(comparison.HolmPValue)) + formatOptionalPValue(comparison.PValue), formatOptionalPValue(comparison.HolmPValue)) if len(comparison.UnpairedSeeds) > 0 { fmt.Fprintf(out, "%d seed(s) usable in one arm only and left out of the pairing: %v\n", len(comparison.UnpairedSeeds), comparison.UnpairedSeeds) @@ -209,6 +209,13 @@ func formatSingletons(summary armSummary) string { return fmt.Sprintf("%d/%d (%.3f)", summary.SingletonDefects, summary.DistinctDefects, *summary.SingletonFraction) } +func formatOptionalPValue(value *float64) string { + if value == nil { + return "n/a" + } + return formatPValue(*value) +} + func formatPValue(value float64) string { switch { case math.IsNaN(value):