fix(metrics): stop reporting Recall@K, F1@K and NDCG that measure nothing - #80
Open
rajarshidattapy wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #67 — four of the six numbers under "RETRIEVAL QUALITY" were either duplicates or
non-standard quantities published under standard names.
Which option I took, and why
The issue offers two routes. I took the second one — stop emitting the degenerate metrics —
rather than wiring up LongMemEval's
has_answer.has_answermarks haystack messages, but retrieval metrics are computed over what aprovider returns: Supermemory memories, Mem0 memories, Zep graph edges and nodes, RAG chunks.
There is no uniform mapping from those artifacts back to source messages, so
has_answercannotproduce a trustworthy
totalRelevantfor the thing actually being scored — and LoCoMo andConvoMem don't ship equivalent labels at all, so it would only ever have worked for one
benchmark. Real relevance labels are a design change, not a bug fix; a wrong recall denominator
would be worse than none.
So this PR makes the reports honest now, and leaves the door open.
Changes
Dropped
recallAtK,f1AtK,ndcgandtotalRelevantfromRetrievalMetrics/RetrievalAggregates, the CLI report, the compare table, and the UI. KepthitAtK,precisionAtK,mrr,kandrelevantRetrieved— all well defined without knowing the corpus.calculateNDCGis deleted as dead code, and the reason is recorded on the type so the triodoesn't get reintroduced by someone reading the field list and noticing gaps.
Judge failures no longer score as "not relevant". Both error paths in
evaluateAllChunksreturned all-zero relevance, making a rate-limit or a timeout indistinguishable from "the
provider retrieved nothing useful" — quietly penalising the provider for the judge's problem.
They now return
null,calculateRetrievalMetricsreturnsundefined, and the question isexcluded from the aggregate instead of contributing a zero. Both paths log a warning.
Note the distinction that had to be preserved: retrieving nothing is still a real zero
(
hitAtK: 0,k: 0), because that's a measurement. Only judge failure is absent. Collapsingthose two would have swapped one silent bias for another.
Collapsed two duplicated tables. Removing three of seven columns from the compare table
meant editing the same width list in four border strings plus two row branches
(
batch.ts), and three near-identical 12-line<td>blocks plus a 7-cell header(
compare/[compareId]/page.tsx). Both are now driven by a single column list, so header and bodycannot drift. That's where most of the −184 lines come from; it wasn't the goal but it was the
cheaper way to make the change correctly.
Tests
src/orchestrator/phases/retrieval-eval.test.tsdrives the real code path against a stublanguage model:
from the output.
1–4" used to both score NDCG 1.0 and Recall 100%. The test shows Hit@K and MRR agree on those
two cases (correctly — both hit, both rank a relevant result first) while Precision now
separates them 25% vs 100%, which is the only honest signal available without labels.
zero.
The stub is hand-rolled (~10 lines) rather than
ai/test'sMockLanguageModelV2, which pulls inmsw— not a dependency here, and not worth adding for one test.bun test5/5,tsc --noEmitclean onsrc.Compatibility
Report JSON and leaderboard rows written before this change still carry the removed fields; they
are simply ignored, so old reports render fine. New reports omit them, and the UI no longer reads
them, so there is no undefined-access path in either direction. No migration needed.
Anyone quoting Recall@K / F1@K / NDCG from a MemoryBench report should treat those figures as
withdrawn rather than changed — they were never measuring what their names implied.
Notes for the reviewer
src/orchestrator/phases/report.tswas already failingprettier --checkat HEAD on apre-existing
basePromptTokensline. I left that line alone rather than bundle a reformat witha metrics change; every line I added is prettier-clean, and the other six files are fully clean.
ui/has 4 pre-existing type errors (LeaderboardEntry.retrieval,promptTokensinquestion-list.tsx). I verified the error set is identical before and after this change, sonone are mine — but they mean
uidoes not currently typecheck, worth its own fix.aggregateRetrievalMetricssetsk: m.kinside thereduce, so the aggregate
kis whichever question came last rather than the run'sk— reportscan still print
RETRIEVAL QUALITY (K=0). It's in a function this PR rewrites, but it's aseparate defect with its own issue, and folding it in would blur what this change is
accountable for.