Repository navigation
Conversation
… and generate report eval and report generate read the same eval file and printed different arm means and deltas. report eval averaged the arms over the complete pairs; report generate averaged each arm over every question scored in it, so the context and baseline means could cover different questions and their difference was not the delta printed beside them. The pipeline run summary had the same per-arm averages. All of them now call eval.stats.paired_summary, which takes the complete pairs only: the arm means, min/max and delta, and the improved/degraded counts. The delta is the difference of the paired means, which equals the mean of the paired deltas. The trust gate and the significance test already used complete pairs. Unpaired questions are counted (paired_questions, unpaired_questions in the JSON; a note and the Count cell in the Markdown), so none drops silently. report eval also rounded each arm mean before subtracting. The delta is now the unrounded difference rounded once for display, so it can move by 0.1. Refs intel#229 (P1-3) Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #250 +/- ##
==========================================
+ Coverage 92.74% 92.76% +0.02%
==========================================
Files 254 254
Lines 32087 32115 +28
Branches 5344 5346 +2
==========================================
+ Hits 29759 29792 +33
+ Misses 1560 1558 -2
+ Partials 768 765 -3
🚀 New features to boost your workflow:
|
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at ac50fc0. The change is right: paired_summary is one definition, the three consumers now agree on arm means and delta, and the changelog and quickstart match the code. Worth fixing before merge: a report page that still contradicts its own Overall table, and test gaps in the grouped tables.
What I ran, on Linux at ac50fc0:
make ci-local: 26 passed, 7 skipped, 2 failed. The two failures are the gitleaks download (the proxy returned 504) and the scan that needs it, not this PR; a direct gitleaks run over the PR's commit found no leaks. Test run and changed-line coverage steps pass.- Reproductions for the findings, with the LLM transport mocked.
- A mutation run of 38 broken copies of the new code against the report, generator, stats, pipeline and record tests: 15 survive.
Findings outside the diff
- Win rate and degradation rate in Key Observations still divide by all questions.
report/eval_report.py:473-476usesall_stats["count"]. With q1 80/60, q2 40/50, q3 context-only and q4 unscored, the Summary row reads4 (2 unpaired) | 60.0 | 55.0 | +5.0and the Key Observations readWin rate: 25% ... (1/4); on the pairs it is 1/2. This predates the PR and the description lists it as out of scope, so it is not a blocker. Usingall_stats["valid"]as the denominator would fix it.
Checked, no issues
- Delta claim:
eval/judge.py:399writesdelta = with - without, so the delta mean over pairs equals the old stored-delta mean for judge-written files, as the changelog says. - Bad scores: NaN, inf, bool,
None, string and one-arm scores throughpaired_summaryall end up unpaired, with no exception. - Pipeline consumers:
commands/benchmark.pyandcommands/orchestrate.pyhandle a missingsummaryand still readwith_avg,without_avganddelta_avg. - Repo rules: a changelog fragment is present, no schema or nav change is needed, and the new tests write only under
tmp_path.
| f"| Min | {stats['with_docs']['min']} | {stats['without_docs']['min']} | {stats['delta']['min']:+.1f} |", | ||
| f"| Max | {stats['with_docs']['max']} | {stats['without_docs']['max']} | {stats['delta']['max']:+.1f} |", | ||
| "", | ||
| ] |
There was a problem hiding this comment.
The Overall table and the top/bottom performer tables now cover different questions. The Max row above is over the complete pairs, but _top_performers and _bottom_performers (generator.py:170-194, outside this diff) still rank every question scored in the context arm, unpaired ones included.
With q1 80/60, q2 40/50 and q3 context 100 with no baseline, the Markdown report prints | Max | 80.0 | 60.0 | +20.0 |, then the note that 1 question is unpaired, and the top-performers table lists q3 | 100.0. A best answer above the stated maximum sends the reader to doubt the table. Before the PR the Max included q3, so the page was consistent.
Suggestion: restrict both lists to questions scored in both arms, as _top_deltas already does, or label them as covering all scored answers.
| def test_both_reports_say_which_questions_the_means_leave_out(tmp_path): | ||
| path = tmp_path / "eval" / "lib.json" | ||
| path.parent.mkdir() | ||
| path.write_text(json.dumps({"evaluations": EVALUATIONS}), encoding="utf-8") | ||
| md = tmp_path / "lib.md" | ||
| generate_report(str(path), str(md), str(tmp_path / "lib.qa.json")) | ||
| eval_md = md.read_text(encoding="utf-8") | ||
| assert "| **All** | 3 (1 unpaired) |" in eval_md | ||
| assert "over the 2 complete pairs; 1 unpaired (scored in one arm only) left out" in eval_md | ||
|
|
||
| data = {"evaluations": EVALUATIONS} | ||
| gen_md = ReportGenerator().generate_report(data, {"questions": []}, "markdown") | ||
| assert "over the 2 complete pairs; 1 unpaired (scored in one arm only) left out" in gen_md |
There was a problem hiding this comment.
The tests pin the Overall and All figures only; the grouped tables can go back to the old formula without a failure. Mutation check: in generator.py _arm_means, changing "with_avg": round(s.with_mean, 1) to round(s.with_mean + 7, 1) passes test_report_arm_agreement.py, test_record_readers.py and test_pipeline_summary.py (113 passed). These mutants also pass the related suites:
_arm_meansusing the old per-arm formula, orunpairedforced to 0 (topic and source rows);_count_celldropped from the cluster, source, Dynamic, Golden, by-type and by-difficulty rows (only theAllrow is asserted);without_minandwithout_maxswapped inpaired_summary, because the fixture's baseline min and max are not asserted;improvementsandneutralswapped ineval_report.stats;- the delta computed from the rounded means in
paired_summary,eval_report.statsor the pipeline, because every fixture mean is a round number.
Tests to add: render the JSON clusters and sources_summary for a fixture where q1 to q3 carry a persona and source_type, and assert with_avg == 60.0, delta_avg == 10.0, unpaired == 1; assert (1 unpaired) in the cluster and source rows of the Markdown; assert (s.without_min, s.without_max) == (40.0, 60.0); and add a pair such as context 60.04 / baseline 50.06, whose delta rounds to 10.0 while the difference of the rounded means gives 9.9.
The agreement fixture has arm means exact at one decimal, so putting back the rounded-difference delta in report eval passed every test. Three pairs with means 80.67/70.33 pin +10.3 in both reports, plus the Breakdown Total row, the delta min/max and the improvement counts. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at f063300. The only change since my last review is a new test that pins the unrounded-mean delta (+10.3 against +10.4 from rounded means), and it does that well. Both of my earlier points are still open, so I would not merge yet.
What I ran, on Linux at f063300:
make ci-local: 28 passed, 0 failed, 7 skipped.- A mutation run of the grouped-table and
paired_summarycode againsttest_report_arm_agreement.py,test_pipeline_summary.pyandtest_record_readers.py: four of five mutants pass all three files.
Still open from my earlier review:
dokimi/dokimi/report/generator.py:316:_top_performersand_bottom_performers(generator.py:170-194, outside this diff) still rank every context-scored question, unpaired ones included, while the Max row above them is over the complete pairs. Unchanged since the first review.dokimi/tests/test_report_arm_agreement.py:70: the new test closes the rounded-difference mutant only. These still pass all three suites:_arm_meanswithround(s.with_mean + 7, 1);_arm_meanswithunpairedforced to 0;_count_cellreplaced by the plain count in the cluster and source rows;without_minandwithout_maxswapped inpaired_summary. The grouped tables (clusters,sources_summary) have no assertion at all.
@xaleryb's question on whether the dashboard should use paired_summary too, or be named as an exception, waits for the author.
Summary
Item #229 P1-3.
report eval,report generateand thepipelinerun summary read one eval file and printed different arm means and deltas. Each now calls one function,eval.stats.paired_summary.Rounding: values are unrounded inside
PairedSummaryand rounded once for display.Paired definition (why). The delta is a per-question comparison, so both arm means must cover the same questions. The trust gate (
gate/trust_gate.py) and the model-compare summaries already use complete pairs. Averaging each arm over all of its questions gives two means over different sets whose difference is not the printed delta (fixture: 73.3 − 50.0 printed as delta 10.0).Evidence
Regression test:
tests/test_report_arm_agreement.py::test_report_eval_and_report_generate_print_the_same_arm_means_and_delta. Fixture: q1 80/60, q2 40/40, q3 context 100 with no baseline (unpaired).pytest tests/test_report_arm_agreement.py):tests/test_pipeline_summary.py, extra one-arm question q4 context 100):tests/test_report_arm_agreement.pyandtests/test_pipeline_summary.pypass (6 passed).8981 passed, 4 skipped.ruff checkclean,mypyclean.Consumers checked by grep:
report/,orchestrator/pipeline.py,commands/benchmark.py(reads the pipeline summarywith_avg),docs/quickstart.md(step 5 wording updated),docs/benchmarking-and-comparison.md,docs/methodology.md.report/model_compare.pyalready averages paired deltas and is unchanged.Internal review: two rounds (author-applied lenses; the subagent review tool was not available in this session). Round 1 found the pipeline summary still used the per-arm formula; fixed in this PR with a test.
Merge Danger
Door: two-way. Code only; no stored format changes. The JSON
statsblock gainstotal_questions,paired_questions,unpaired_questionskeys and the pipeline summary keys are unchanged. Reverting restores the old numbers.Numbers change (say which, and how):
report generate(and thepipelinereport): Overall context / baseline means, min/max, topic and source means, and the Count cell change for any eval file with an unpaired question (scored in one arm only). For a file the judge wrote, the delta does not change, because the stored delta already was the paired difference. Files where every question is paired do not change.pipelinerun summary (printed line,steps.evaluation.summary, andbenchmark runaverages read from it): same rule, complete pairs.report eval: arm means print as before. The Delta is the unrounded difference rounded once, so it can move by 0.1 (rounding probe: about 22% of random cases differ by 0.1). The Count cell shows unpaired questions and a note explains the means.Blast radius: report output and the pipeline summary for files with unpaired questions. Results recorded before this change keep their old numbers in their own reports; rerun
report generateon an existing eval file to get the new ones. No judge, answer or eval-file writes change.Out of scope, found and not changed: eval_report win-rate and degradation percentages divide by all questions while numerators are over pairs; generator top/bottom delta lists read the stored per-record delta (equal for judge-written files); Key Observations thresholds use rounded display values.
Refs #229
🤖 Generated with Claude Code