Milestone 4: quality — the graph signal is measured, and the answer is no - #7
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af32efbec9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Eight findings from the Codex review of #7, all confirmed against the code before fixing. Two of them changed published measurements, so section 5 of docs/EVAL.md was measured again end to end. The one that mattered: `weft-eval build` inverted the Semantic Scholar cache into CorpusId -> cord_uid by ranging over a Go map. That mapping is not injective — CORD-19 ships the same paper under several cord_uids, and 20,556 of 162,837 records with a CorpusId collide — so randomised map iteration chose a different winner on every build. Two builds from the identical cache disagreed on 2,571 to 9,377 of 142,281 CorpusIds, up to 6.6% of the citation graph. The edge count was identical every time, 579,720, which is why the build log looked stable and nothing downstream noticed. Nothing guarding the measurement could see it: the bootstrap resamples queries against one index, the seed pins the resampling, the 28-configuration sweep varies fusion. A harness whose purpose is reproducibility had never been asked to reproduce anything. Two consecutive builds now produce byte-identical segments. The verdict survived. The binding delta went from -0.1156 to -0.1202, CI [-0.1521, -0.0886], still excluding zero, still 0 sign flips across the sweep. What did not survive is the weight sweep's headline: the graph's best case under any fusion weight was published as +0.0018 and is +0.0000. That figure was smaller than the run-to-run spread of the graph it was measured on, and it was the one number a reader might have taken as a reason to keep the scorer. No weight beats the baseline; from 0.1 downward the arm is the baseline. - P1 build: CorpusId collisions resolved in sorted key order, first wins, collision count printed. Extracted as corpusIDIndex with a test that runs it 200 times, because map iteration order is randomised per range statement and one comparison would pass with the bug present. - P1 prepare: json.Decoder.InputOffset stops at the closing brace, one byte before the newline the encoder wrote, so a healthy cache looked like it carried a one-byte fragment and the resume truncated its own record separator. Go's stream decoder reads `}{` back happily; the line-oriented gen_query_vectors.py --verify does not. - P1 run: a query-vector file that loads and matches nothing left withVec at 0, indistinguishable from no file, and published a text-only run under the text+vector label. - P2 prepare: documents with no joinable identifier were never recorded, so every rerun rescanned 1.6 GB of metadata to rediscover it and then failed with ErrEmptyDataset once the joinable ones were all cached. They now get a key-only record; the cache holds one entry per corpus document. - P2 bootstrap: percentileIndex truncated p*n instead of taking ceil(p*n)-1, so both bounds were one order statistic high. Every interval here reflects the correction; it moves the third or fourth decimal and changes no reading. - P2 fusion: FuseWeighted accepted +Inf, which sends every document in the stream to +Inf where they compare equal and TopK ties them on DocID — the same collapse to insertion order the NaN guard exists to prevent. The test that had pinned the opposite behaviour passed only because its fixture ranked in DocID order. - P2 diagnose: -k=0 indexed cands[-1] and panicked. The other subcommands were covered by eval.Evaluate's guard; diagnose reads streams directly. - P2 s2: the retry loop slept the full backoff after its final attempt, adding up to two minutes to a failure already decided. docs/EVAL.md section 5.12 and docs/FINDINGS.md section 4.2 record the determinism failure next to section 4.1's withdrawn finding — same shape, one layer lower: a statistic answering the question it was asked while the thing that moved the number sat outside its scope.
Eight findings from the Codex review of #7, all confirmed against the code before fixing. Two of them changed published measurements, so section 5 of docs/EVAL.md was measured again end to end. The one that mattered: `weft-eval build` inverted the Semantic Scholar cache into CorpusId -> cord_uid by ranging over a Go map. That mapping is not injective — CORD-19 ships the same paper under several cord_uids, and 20,556 of 162,837 records with a CorpusId collide — so randomised map iteration chose a different winner on every build. Two builds from the identical cache disagreed on 2,571 to 9,377 of 142,281 CorpusIds, up to 6.6% of the citation graph. The edge count was identical every time, 579,720, which is why the build log looked stable and nothing downstream noticed. Nothing guarding the measurement could see it: the bootstrap resamples queries against one index, the seed pins the resampling, the 28-configuration sweep varies fusion. A harness whose purpose is reproducibility had never been asked to reproduce anything. Two consecutive builds now produce byte-identical segments. The verdict survived. The binding delta went from -0.1156 to -0.1202, CI [-0.1521, -0.0886], still excluding zero, still 0 sign flips across the sweep. What did not survive is the weight sweep's headline: the graph's best case under any fusion weight was published as +0.0018 and is +0.0000. That figure was smaller than the run-to-run spread of the graph it was measured on, and it was the one number a reader might have taken as a reason to keep the scorer. No weight beats the baseline; from 0.1 downward the arm is the baseline. - P1 build: CorpusId collisions resolved in sorted key order, first wins, collision count printed. Extracted as corpusIDIndex with a test that runs it 200 times, because map iteration order is randomised per range statement and one comparison would pass with the bug present. - P1 prepare: json.Decoder.InputOffset stops at the closing brace, one byte before the newline the encoder wrote, so a healthy cache looked like it carried a one-byte fragment and the resume truncated its own record separator. Go's stream decoder reads `}{` back happily; the line-oriented gen_query_vectors.py --verify does not. - P1 run: a query-vector file that loads and matches nothing left withVec at 0, indistinguishable from no file, and published a text-only run under the text+vector label. - P2 prepare: documents with no joinable identifier were never recorded, so every rerun rescanned 1.6 GB of metadata to rediscover it and then failed with ErrEmptyDataset once the joinable ones were all cached. They now get a key-only record; the cache holds one entry per corpus document. - P2 bootstrap: percentileIndex truncated p*n instead of taking ceil(p*n)-1, so both bounds were one order statistic high. Every interval here reflects the correction; it moves the third or fourth decimal and changes no reading. - P2 fusion: FuseWeighted accepted +Inf, which sends every document in the stream to +Inf where they compare equal and TopK ties them on DocID — the same collapse to insertion order the NaN guard exists to prevent. The test that had pinned the opposite behaviour passed only because its fixture ranked in DocID order. - P2 diagnose: -k=0 indexed cands[-1] and panicked. The other subcommands were covered by eval.Evaluate's guard; diagnose reads streams directly. - P2 s2: the retry loop slept the full backoff after its final attempt, adding up to two minutes to a failure already decided. docs/EVAL.md section 5.12 and docs/FINDINGS.md section 4.2 record the determinism failure next to section 4.1's withdrawn finding — same shape, one layer lower: a statistic answering the question it was asked while the thing that moved the number sat outside its scope.
6a686a3 to
814e366
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 814e3664dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73a7e9c88f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae7c853ed8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2632eddb08
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f70fdb918b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4017fafb82
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdbd60af07
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…o the one asked PR #7 review, six findings. The shape they share is narrower than the earlier rounds': in each one a check exists, runs, and passes — on a quantity that is adjacent to the one the rule names. The evidence was about another corpus, the sign came from the interval instead of the delta, the pinned files were not the input being pinned. eval/prepare: the evidence that a join can work was counted over the whole cache. It is the guard that separates "every joinable document is already cached" from "this metadata release does not match this corpus", and its evidence is a cached record carrying a CorpusId — counted without asking whose corpus the record's key belongs to. An s2.jsonl kept from an earlier dataset supplies it: none of the current keys match the metadata, the guard sees a working join and stands down, and every current document is written as a tombstone. build reads that as complete coverage and publishes vector and graph arms over an index with neither, which is the docs/EVAL.md section 4.1 failure rebuilt out of the check meant to stop it. The tally is now scoped to the corpus being prepared; keys, models and the resume offset still describe the file. eval: the corpus-pairing check skipped the nonrelevant judgments. idealDCG drops everything at or below 0, so a missing grade-0 document moves no term of the sum — true of the arithmetic, false of the ranking. A judged-nonrelevant document is one an assessor saw because a system retrieved it, so in the corpus those judgments belong to it takes a slot above the cut that something else takes here. It is also the same evidence of a mispaired qrels file and index as a missing relevant one. Measured before tightening rather than assumed safe: all 66,336 of trec-covid's judgments, including the 41,663 at grade 0, name documents in the BEIR corpus, so the stricter check refuses no run that was measuring correctly. eval-cmd: run, sweep and weights verified the files they read and not the index they measure. queries.jsonl and qrels/test.tsv are pinned; the index is a third input and nothing described it. `build -any-snapshot` against another corpus revision, followed by `run` beside the pinned judged inputs, therefore passed every check there was — and a revision keeping the same document keys satisfies the qrels check too, so different text, vectors and links get printed under the published labels. build now writes index/provenance.json recording the sha256 of the corpus it read, hashed at build time whether or not the snapshot check ran, and whether -partial was used; run, sweep, weights and diagnose refuse an index that does not match, cannot say, or was built over a cache not covering the corpus. -any-snapshot opts out, as everywhere else. eval-cmd: sweep took rule 2's sign from the confidence interval. Section 4 rule 2 is the sign of the observed delta and nothing else; the interval is rule 1's criterion at the frozen point. Reading them off one value made a cell whose interval spanned zero signless, and the flip count skipped it — so a grid going +0.02, -0.02, +0.02 under wide intervals established no first sign, counted no flip, and printed "rule 2 holds" about deltas that had flipped twice. The sign now comes from the delta, the delta gets its own column, and cells whose interval spans zero are counted and reported beside the verdict instead of being folded into it. eval: ReadCorpus streamed a duplicate _id without noticing. ReadQueries has refused a repeated query id all along, and this is the same fault at the more expensive end: prepare puts the key in its fetch list twice, asks Semantic Scholar about it twice, and writes two records under one key — which build rejects, after the hours of rate-limited fetching that produced them. The ids are now held and checked; a set of 171K keys is a few megabytes beside the documents the streaming exists to avoid holding. eval-cmd: -iters=0 was rejected after every arm had been evaluated. BootstrapCI refuses it correctly and is reached last, so on the documented corpus that is 50 queries against 171,332 documents with a brute-force vector scan each, spent to arrive at a flag error decidable before the index was opened; sweep is worse, at three arms per grid cell. Checked immediately after parsing in all three commands, with an empty -data directory in the test so a check that drifts back below the index open fails on a missing file instead. Each fix has the regression that fails without it. make all 7/7, make arch 7/7 including the public API golden, make deps unchanged. The published numbers are being re-measured in the same working tree and will follow in their own commit: the previous round's duplicate-reference dedup collapsed exactly one edge, 579,720 to 579,719, and that moved the binding delta from -0.1202 to -0.1227. Nothing in this round touches a ranking.
…ify them PR #7 review, second pass, three findings. Each accepts a value on the strength of something that looks like identity and is not: a vector's width, a flag's sign, the ratio between two weights after rounding. fusion: scaling the weights could reorder the fusion it was documented not to reorder. scaleDown divides every weight by the largest when that exceeds 1, on the argument that only the ratios affect the fused order. True of the arithmetic and not of float64: the division rounds, and it rounds a document's one term differently from another document's two. At weights 3, 2, 1 and RRFk=60 a document at rank 152 of the first stream outscores one at ranks 92 and 947 of the other two by one ulp; divide by 3 and the totals become equal, at which point TopK settles them on DocID — the collapse to insertion order every other guard in that file exists to prevent, reached through the one operation that promised to change nothing. Scaled by a power of two now, via Frexp and Ldexp, which adjusts the exponent and leaves the significand alone: every score comes out as the unscaled score over 2^exp to the last bit, so the order cannot move, and the overflow bound the scaling exists for is slightly tighter than before (maxW lands in [0.5, 1)). No weight in this repository exceeds 1, so scaleDown does not fire and no published figure moves. eval/build: a vector was indexed because its width matched. SPECTER v1 and v2 both emit 768 dimensions, so dominantDim accepts either and engine.Add stores either, while the query vectors come from gen_query_vectors.py under eval.S2Model. Cosine similarity across two embedding spaces is not a similarity, and it arrives as a plausible vector baseline with the whole graph delta measured against it. Nothing downstream could catch it: a committed index carries no model label, and prepare's warning belongs to whichever invocation fetched the batch, which on a resumed job is not the one that finishes. build now tallies the models behind the corpus's cached vectors and refuses a foreign one, with -partial as the override — the same flag that already means "these arms are not publishable", and which run, sweep and weights refuse by the index's provenance. An unrecorded model is warned about and indexed: that is what the committed measurement was fetched into, all 148,232 of its vectors, so refusing it would refuse the published index rather than a mistake. eval/prepare: -limit=-1 meant unlimited. 0 is the documented unlimited value and the guard read `limit > 0`, so a smoke-test typo started fetching the entire corpus — hours of rate-limited requests against a shared anonymous budget, appending to the resumable cache the whole way, with nothing in the log saying the flag had been ignored. Rejected before the corpus is read and before anything is written. Each fix has the regression that fails without it; the fusion one was run against the unfixed scaleDown and reports the tie it produces. make all 7/7, make arch 7/7 including the public API golden, make deps unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5e8529514
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdf78aebdc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…bes could disagree PR #7 review, third pass. Three of these are provenance — a record that could outlive what it describes, an artifact that recorded nothing, a flag parse that discarded what it had been given — and the fourth is a sentence claiming more than the grid under it measured. eval/build: the provenance record could outlive the index it describes. A rebuild replaced the segments and then wrote provenance.json, and a crash in that window left the previous record standing beside a manifest it does not describe. A later run verifies the stale record and accepts a foreign or partial index as the pinned one — the substitution provenance exists to refuse, reached through provenance itself. The old record is now removed before the commit, so an unfinished rebuild leaves an index that cannot say what it holds, which is the answer verifyProvenance already refuses. The regression fails a commit on a corrupt manifest and asserts nothing describes the index afterwards. eval: the query vectors recorded no model, while the document side had just started refusing one. Same hazard from the other end — a file generated from another adapter, base revision or local model configuration carries the right query id and the right question text, so the id check and the text check both pass and the vector scorer computes cosine similarity between two embedding spaces. gen_query_vectors.py now writes the model into every record, and loadQueries refuses one that names anything but the SPECTER2 base plus the ad-hoc query adapter. Unrecorded is tolerated on both sides, for the same reason: the committed query-vectors.jsonl and all 148,232 committed document vectors predate their field, so refusing absence would refuse the published measurement rather than a mistake. eval-cmd: a positional typo silently discarded every flag after it. flag stops parsing at the first non-flag argument and reports nothing, so `weft-eval prepare typo -limit=1` runs with -limit at its default — and the default means unlimited, which is hours of rate-limited API requests appending to the resumable cache with the flag meant to keep it small thrown away. dataFlags now refuses a leftover argument; no subcommand takes one. docs: "no weight beats the baseline" was a claim about eight sampled weights. nDCG is not monotonic in the fusion weight — a weighted RRF ranking changes at query-specific score-crossing thresholds — so the ends of an unsampled interval do not bound what happens inside it. Split into the two claims that were being run together: at 0.1 and below the arm is bit-identical to the baseline, so every weight in that region provably changes nothing; above it, the honest statement is that no *tested* weight beats the baseline, with 0.25-to-0.1 named as where an untested value would have to hide. EVAL 5.11 states it in full and says why it is a thin hope rather than an open question; README, FINDINGS and the verdict table now say "in the tested grid". No number moves — this is what the numbers were already entitled to mean. Each fix has the regression that fails without it. make all 7/7, make arch 7/7 including the public API golden, make deps unchanged. No published figure changes.
…not been re-measured PR #7 review, fourth pass, two findings. eval-cmd: judgments for a query the query file does not hold were unreachable. loadQueries walks the queries and looks each one's judgments up, so a qrels row naming a query absent from queries.jsonl is not skipped, not counted and not reported — it is never reached. A truncated or mismatched query file therefore yields a mean and a bootstrap over however many queries survived, printed under the usual heading with a query count nobody compares to 50. Dropping a judgment-less query is the deliberate case and is already counted; this is its mirror image and there is no reading of it that is not a broken pairing, so it is refused, with the offending id named in sorted order. Checked even under -any-snapshot: that flag says "a different corpus", not "a query set and its judgments that disagree". Measured before tightening — all 50 of trec-covid's qrels query ids are in queries.jsonl and all 50 queries are judged — so it refuses no run that was measuring correctly. docs: the exported Go documentation still published the pre-re-measurement numbers. pkg/scorer/graph's package doc is where a user of the library meets this verdict, and it named 579,720 edges and −0.1202; pkg/fusion/rrf.go repeated −0.1202 twice as the motivation for FuseWeighted, and weights' own doc comment carried it as the number it exists to explain. Someone reading `go doc` saw evidence disagreeing with the repository's published result. Updated to 579,719 and −0.1227 alongside README.md and docs/EVAL.md; the +0.0000 best case and the 0.0019 recovered by halving the weight are unchanged, so no sentence around them needed rewriting. make all 7/7, make arch 7/7, make deps unchanged. `weft-eval run` against the committed index still reproduces every figure in docs/EVAL.md section 5.9 with all of this round's and the previous rounds' checks in force — including the provenance gate, which the rebuilt index passes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70af775c84
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
PR #7 review, fifth pass, two findings. One is a published number that was describing something other than what it was labelled; the other is a property worth pinning rather than a regression. eval/diagnose: "slots decided by DocID" counted the candidates that lost, not the slots they lost. A tie group straddling the cut was tallied by how many cut-score candidates fell *outside* the top k, and the summary called that total "slots decided by DocID". The two come apart badly: 100 candidates tied for 10 positions is 90 excluded and 10 arbitrary slots, and reporting 90 makes a claim about a top-10 that a top-10 cannot hold. The published figure was of that kind — 960 across 45 rankings of 10. Both counts are now measured and printed, because they answer different questions: how large the tie group was, and how much of the reported answer it decided. On the current index, 41 of 45 queries have a tie group crossing the cut, **241 of the reported slots are held at the cut score, and 960 further candidates are excluded from it by DocID alone.** docs/EVAL.md section 5.9's table row is relabelled rather than corrected — 2,082 and 960 were always the excluded count, in both the before and after columns, so they are comparable and stay. The slots figure is a new row, with "not measured" for the pre-fix column: it would take reverting the graph scorer to obtain, and nothing in the argument needs it. Section 5.13 and FINDINGS §5 now say both numbers where they used to say one. fusion: the underflow at the far end of the weight range is documented and pinned, not changed. The report reads as a regression from the power-of-two scaling of round 9 and is not one — 1e-300 against math.MaxFloat64 underflows to zero identically under `w /= maxW` and under `Ldexp(w, -exp)`, because the ratio is past what float64 holds either way. What the report is right about is the consequence, which the comment had glossed: weight 0 means fuse does not create the entry, so the stream's documents are absent from the result rather than last in it. Stated exactly now, with the reason the trade goes this way — overflow takes the ranking apart for every stream at once, underflow costs the one stream that asked to be 1e308 times quieter than another — and TestScaleDownDropsAStreamItCannotRepresent pins both sides, including that a representable ratio still ranks the quiet stream rather than dropping it. make all 7/7, make arch 7/7, make deps unchanged. No arm number, interval or sign moves: diagnose reports on the graph stream before fusion and is not an input to any of them.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a494609a5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
PR #7 review, sixth pass, two findings. Both are about a guard that was satisfied by something weaker than the thing it was guarding. fusion: a document's weighted votes were summed in stream order. Rank-major accumulation makes a total a function of the ranks a document earned rather than of the order the scorers were listed in, and within one rank that used to rest on "repeats of a rank add the same value and commute" — true unweighted, false with weights. Three streams meeting at rank 1 with weights 0.25, 1/6 and 0.125 sum to one of two different last bits depending on the order they are visited, and against a competitor weighted 0.5416666666666666 that bit is the difference between winning outright and tying and losing on DocID. Moving a scorer and its weight together is the same fusion and could produce a different ranking. Streams are now visited lightest weight first within each rank, so the sum is a function of the multiset of votes a document actually earned. The sort is stable, so equal weights keep slice order and the unweighted path — every ranking milestones 1 and 2 pin — is bit-identical. TestFuseWeightedIsInvariantToStreamOrder enumerates all 24 permutations of the four (stream, weight) pairs above and was run against the previous accumulation, where it fails. The weight sweep was re-measured because it is the one published table that fuses with weights: every cell is identical, including the -0.0019 at 0.5 and the exact +0.0000 from 0.1 down. docs/EVAL.md section 5.11 needs no edit. testdata: the query-vector sanity check passed on a bare majority of wins. It compares each query's mean cosine to its judged-relevant documents against its mean cosine to random ones, and accepted any result with more wins than losses — which for 50 queries is 26, the median outcome of a coin flip. An adapter embedding into the wrong space produces exactly that, so the check would have admitted the case it exists to refuse, and the aggregate cosines printed immediately above it were never compared at all. It now requires a one-sided sign test at p < 0.001, computed exactly with math.comb so the script stays on the standard library, and a positive aggregate margin. The recorded run — 50/50 wins, p = 8.9e-16, means 0.7620 against 0.6842 — clears both by a wide margin; 34 of 50 wins would fail. docs/EVAL.md section 5.4 records the p-value and why the bar moved. make all 7/7, make arch 7/7, make deps unchanged. No published figure moves.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae368fa20b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…s no Graph proximity does not improve ranking. On TREC-COVID joined to the Semantic Scholar citation graph — 171,332 documents, 579,720 in-corpus edges, 148,232 SPECTER2 vectors, 50 queries — adding it costs 0.1156 nDCG@10 (95% CI [-0.1483, -0.0837]) against the pre-registered text+vector baseline, and the sign does not flip across 28 configurations of the RRF rank constant and fusion depth. The PRD's second falsification condition is met and answered no. The larger finding is about fusion, not about graphs. That -0.1156 belonged to RRF's equal vote: halving the graph stream's weight erases the entire regression, and at the best weight the signal is worth +0.0018 with an interval touching zero. So the accurate statement is the flatter one — the graph signal is not harmful information, it is not information. - internal/eval: nDCG@10, an arm runner, a paired bootstrap and the dataset readers. The harness never learns what a graph scorer is: an Arm is a name, a []engine.Scorer, a Fuser and a depth, and five arms differ only in the contents of a slice. Standard library only; `make deps` still prints one module. - Both instruments were checked against outside implementations before any arm number existed. That caught the plan's own nDCG definition: trec_eval uses linear gain, not the exponential form the plan specified, and publishing on the wrong scale would have made every figure incomparable. BM25 agrees with rank_bm25 to 4.44e-16, closing the PRD's long-unclaimed correctness floor. - fusion.FuseWeighted: per-stream weights indexed by position, not scorer kind — the caller already fixed that order — so fusion still learns nothing about what it holds and `go list -deps ./pkg/fusion` still names no scorer. Fuse is unchanged and its unweighted path is bit-identical, so no ranking pinned by milestone 1 or 2 moved. - scorer/graph: per-seed distances are summed rather than taking the nearest, which the plan named in advance as the fix if the degeneracy diagnostic bit. It bit — 38 of 50 queries had their top k decided by DocID, i.e. insertion order — and the fix recovered +0.044 of the 0.12 needed. Both measurements are kept. - The scorer is kept rather than deleted, on the record and against the PRD's literal instruction: it is inert rather than harmful, and removing it would cut the milestone 1 assertions from four signals to three. Its package doc now opens with the measurement and the instruction to weight it down. D-005 argues both sides and names the signal that would show the choice was wrong. - Over-fetching needed no engine API after all: Fuse scores from ranks alone, so Search(ctx, q, k*m, ...) truncated to k equals fusing to k. The ponytail marker at search.go:112 is withdrawn rather than repaid. - docs/EVAL.md is the provenance document, including section 4.1: a finding this milestone published to itself on 27% coverage, with a 95% interval of [-0.3058, -0.2178], and then withdrew. Narrow, far from zero, and wrong. The bootstrap quantifies sampling noise and says nothing about whether the corpus is complete. make all, make arch (7/7) and make deps are green; `make eval` reprints every published figure from a committed index.
Eight findings from the Codex review of #7, all confirmed against the code before fixing. Two of them changed published measurements, so section 5 of docs/EVAL.md was measured again end to end. The one that mattered: `weft-eval build` inverted the Semantic Scholar cache into CorpusId -> cord_uid by ranging over a Go map. That mapping is not injective — CORD-19 ships the same paper under several cord_uids, and 20,556 of 162,837 records with a CorpusId collide — so randomised map iteration chose a different winner on every build. Two builds from the identical cache disagreed on 2,571 to 9,377 of 142,281 CorpusIds, up to 6.6% of the citation graph. The edge count was identical every time, 579,720, which is why the build log looked stable and nothing downstream noticed. Nothing guarding the measurement could see it: the bootstrap resamples queries against one index, the seed pins the resampling, the 28-configuration sweep varies fusion. A harness whose purpose is reproducibility had never been asked to reproduce anything. Two consecutive builds now produce byte-identical segments. The verdict survived. The binding delta went from -0.1156 to -0.1202, CI [-0.1521, -0.0886], still excluding zero, still 0 sign flips across the sweep. What did not survive is the weight sweep's headline: the graph's best case under any fusion weight was published as +0.0018 and is +0.0000. That figure was smaller than the run-to-run spread of the graph it was measured on, and it was the one number a reader might have taken as a reason to keep the scorer. No weight beats the baseline; from 0.1 downward the arm is the baseline. - P1 build: CorpusId collisions resolved in sorted key order, first wins, collision count printed. Extracted as corpusIDIndex with a test that runs it 200 times, because map iteration order is randomised per range statement and one comparison would pass with the bug present. - P1 prepare: json.Decoder.InputOffset stops at the closing brace, one byte before the newline the encoder wrote, so a healthy cache looked like it carried a one-byte fragment and the resume truncated its own record separator. Go's stream decoder reads `}{` back happily; the line-oriented gen_query_vectors.py --verify does not. - P1 run: a query-vector file that loads and matches nothing left withVec at 0, indistinguishable from no file, and published a text-only run under the text+vector label. - P2 prepare: documents with no joinable identifier were never recorded, so every rerun rescanned 1.6 GB of metadata to rediscover it and then failed with ErrEmptyDataset once the joinable ones were all cached. They now get a key-only record; the cache holds one entry per corpus document. - P2 bootstrap: percentileIndex truncated p*n instead of taking ceil(p*n)-1, so both bounds were one order statistic high. Every interval here reflects the correction; it moves the third or fourth decimal and changes no reading. - P2 fusion: FuseWeighted accepted +Inf, which sends every document in the stream to +Inf where they compare equal and TopK ties them on DocID — the same collapse to insertion order the NaN guard exists to prevent. The test that had pinned the opposite behaviour passed only because its fixture ranked in DocID order. - P2 diagnose: -k=0 indexed cands[-1] and panicked. The other subcommands were covered by eval.Evaluate's guard; diagnose reads streams directly. - P2 s2: the retry loop slept the full backoff after its final attempt, adding up to two minutes to a failure already decided. docs/EVAL.md section 5.12 and docs/FINDINGS.md section 4.2 record the determinism failure next to section 4.1's withdrawn finding — same shape, one layer lower: a statistic answering the question it was asked while the thing that moved the number sat outside its scope.
…mber All six are the same shape as the milestone's own worst failure — a run that completes, prints a table, and labels arms for signals that were not there. - build refuses an incomplete Semantic Scholar cache. prepare writes a tombstone even for documents with no S2 side, so a finished run covers the corpus exactly and a gap means it did not finish; those documents entered the index text-only while the run still reported text+vector+graph. That is EVAL 4.1 reproducible on demand. -partial keeps the deliberate slice build, loudly. - S2Record carries the embedding model. The tally lived only in the invocation that fetched a batch, so an interrupted prepare took provenance with it and the run that finished the job reported its own tail as clean. doneKeys now tallies the whole cache; a record predating the field reads as unrecorded, not wrong. - FuseWeighted scales weights down when the largest exceeds 1. Each sw/(RRFk+rank) term stays finite near MaxFloat64 but ~62 streams overflow the sum to +Inf, and every commonly ranked document ties there and collapses onto DocID — the NaN and +Inf guards' failure, reached from entirely in-contract weights. Ranking depends only on ratios, so this is order-preserving, and every weight set in the repo has a maximum of 1 and is untouched. - -rrfk is validated. -rrfk=-1 divides by zero at rank 1 and printed the result as nDCG under a heading naming the constant. - An all-zero query vector is rejected. The vector scorer reads a zero norm as no opinion, so full coverage was reported for a file under which text+vector is exactly text. - Query vectors are paired by id and verified by text. Ids are stable across regenerations of the query set, so a file from an older snapshot matched every id and evaluated different questions under the current qrels. The generator has always written the text; nothing read it. No published number moves. Re-ran the frozen arms, the weight sweep and the determinism check against the real 171,332-document corpus: identical to the last commit, including -0.1202 CI [-0.1521, -0.0886] and the +0.0000 best weight. The cache covers 171,332 of 171,332 keys, so the new build gate passes without -partial.
Five findings, all real, all in the same family as rounds 1 and 2: an input that is wrong in a way nothing compares, producing a table with arm names on it. - prepare treated "no cord_uid matched" as the normal end of a resumed run. On a fresh run against the wrong metadata release it is not: it recorded the entire corpus as asked-and-unjoinable, which satisfied round 2's coverage gate with a cache that never joined once. Refused now unless a record in the cache carries a CorpusId, which is the only evidence in the file that this join can work at all. - build inverted CorpusId -> cord_uid over every record in the cache, so a stale key from a larger or older corpus that sorted below the present copy won the mapping and every citation to that paper resolved to a document the index does not hold — real in-corpus edges dropped as dangling, with the coverage gate satisfied because the cache did cover the corpus. The mapping is now built from in-corpus keys only, and the rest are counted and reported. - Evaluate scored qrels it never checked against the index. A judged-relevant document the index does not hold cannot surface as a wrong lookup: it stays in IDCG, where every arm loses part of its score, and a stale index/qrels pairing reads as a ranking that got worse. - ReadQrels kept whichever grade came last for a repeated query/document pair, so concatenating two assessment rounds made the ground truth a function of row order. Conflicting grades are refused; an identical repeat still is not. - ReadQueryVectors let a repeated id overwrite its vector, where both lines carry the same text and neither the pairing check nor the coverage count can tell them apart. doneKeys became scanS2Cache returning a struct: the join evidence was the fifth thing one pass over the cache had to report. No published number moves. The rebuilt index is byte-identical in every segment payload (docs, meta, postings, terms) and the frozen arms reproduce exactly: 0.5826 / 0.6233 / 0.3987 / 0.5031 / 0.5464, binding delta -0.1202 95% CI [-0.1521, -0.0886]. The real inputs pass every new gate — 66,336 qrel rows with no conflicting duplicate, 35,480 judged documents all in the index, 50 distinct query vectors, and a cache holding exactly the 171,332 corpus keys.
Seven findings. The first is the one that mattered: `sweep` measured text+graph against text and then printed "docs/EVAL.md section 4 rule 2 holds", but rule 2 names text+vector+graph against text+vector. Both the code comment and EVAL 5.10 admitted the gap and called re-running it an hour's work. It is 14 minutes, and it is now measured: 28 configurations of the binding pair, 0 sign flips, no interval reaching zero, closest approach [-0.0380, -0.0065] at RRFk=60 over-fetch 10. The verdict is unchanged and now rests on the comparison it names. The binding grid also shows what the text-only grid could not: over-fetching lifts the baseline arm from 0.6233 to 0.7091, because both of its streams deepen where a single-stream baseline is flat by construction. The gap narrows from 0.1273 to 0.0218 and never closes — it narrows by lifting both arms. Six input and claim fixes: - prepare tombstoned every unseen document whenever the metadata scan matched at least one key, so a truncated 1.6 GB download recorded a corpus as permanently unjoinable and build read that as full coverage. Round 3's guard only covered zero matches. The four downloaded inputs are now pinned by size and sha256 (-any-snapshot opts out), checked before prepare writes and before run, sweep, weights or diagnose prints. Measured while sizing this: every corpus document present in the 2022-06-02 release has an identifier, so the 6,140 unjoinable ones are simply absent from it — which is why a truncated file cannot be detected from the join alone. - A qrels file truncated at a row boundary read as a clean prefix and lifted every arm by an unreported amount. Same pin. - readS2Records let a repeated key overwrite silently, so a tombstone concatenated after a full record discarded a vector and a reference list while coverage still counted the document. - gen_query_vectors.py --verify sampled before checking corpus membership, so a cache from a larger corpus could sample only foreign keys and crash on an empty similarity list instead of verifying anything. - ErrForeignDocID's comment, EVAL 2.2 and FINDINGS all described a bound check as validating cross-index arms. It is not: ids are dense from zero, so a foreign index of similar size resolves in range to unrelated documents. The claim now says what the check does. - diagnose accepted -deep <= -k, which truncates the tie group at the cut being measured and reports zero arbitrary slots for every query whatever the graph did. Rebased onto main (4a6b28e); README's conflict resolved keeping both sides, FuseWeighted recorded in main's new public_api.txt golden, line counts refreshed. No published number moves. The rebuilt index is byte-identical in every segment payload, the frozen arms and the weight sweep reproduce exactly (-0.1202 CI [-0.1521, -0.0886], best weight +0.0000), the tie analysis still reports 41 of 45 and 958 slots, and the sweep's comparison column reproduces the -0.1762 to -0.1895 range this section published before the binding pair was added.
…they guard Four findings, all P2, all the same shape. In each one a check exists, is correct about the case it names, and lets a neighbouring case through to the exact failure the check was written to prevent. - FuseWeighted(2) over two streams returned the unweighted fusion. A stream with no weight of its own is documented to count 1.0, and scaleDown divided only the weights it was handed, so the requested 2:1 was stored as 1:1 — which is Fuse, the one fusion a caller reaching for FuseWeighted is asking not to get. The implicit weight now scales with the rest: scaleDown returns what a stream past the end of the slice weighs, and fuse takes it as the default. Every weight set in this repository has a maximum of 1, so scaleDown does not fire and no published figure moves. - A weight below about 1e-322 is positive, finite and entirely in contract, and its vote still underflows to zero at every rank. `+=` created the map entry anyway, so the stream's documents arrived at score 0, tied, and TopK settled them on DocID: the collapse to insertion order the NaN and +Inf guards exist to stop, reached from a value none of them reject. The test is now on the product rather than the weight, which is also the only form that stays correct if RRFk or the fusion depth changes. - scanS2Cache read damage in the middle of s2.jsonl as a half-written tail. json.Decoder reports both as one error, and prepare's response to a short offset is os.Truncate, so a damaged record with valid ones behind it deleted every record that followed — hours of rate-limited fetching — logged it as an incomplete trailing record, and the next run refetched what it no longer had. The encoder writes one record per line, so a record that never finished writing is the last line and holds no newline; a newline past the last good offset now refuses the scan instead of offering the truncation. Scanned in a fixed buffer, because the damaged case is the one with no bound on it. - gen_query_vectors.py concluded the vector arm was noise and returned success. The file was already written, so `weft-eval run` picked it up and reported a vector arm — the baseline the entire graph delta is measured against — under its usual name. The check now runs before the file exists and exits 1 without writing. A check that could not run (no cached document vectors, no judged document among them) still passes: refusing to write before `weft-eval prepare` has run would break the pipeline's actual order. make all 7/7, make arch 7/7 including the public API golden, make deps unchanged. The graph weight grid runs from 1 to 0.001 against a maximum of 1, so neither fusion change reaches a call site: docs/EVAL.md is untouched.
Each of these accepted something as given and only found out later that it
was not. The shape is the same one the earlier rounds kept finding, one layer
further in: a value taken on faith at the point it entered, and a confident
number published on the far side of it.
fusion: a weight with no stream still set the scale. scaleDown ran once in
FuseWeighted, over every weight it was handed, because that is the only place
the slice was known — the stream count is not. So a surplus weight, documented
as ignored, still set the maximum every active weight was divided by.
FuseWeighted(1e-320, math.MaxFloat64) over one stream underflowed that stream
to zero and returned nothing. Scaled per invocation now, over the prefix the
streams actually claim.
graph: the score depended on the order the seeds were listed in. Float
addition is not associative, and the sum accumulated per seed in caller order,
so a document at hops {1,1,1,2,3} came to 2.083333333333333 or
2.0833333333333335 depending on which seed came first. Two documents holding
the same distances — which the formula says tie — then came out unequal, and
TopK's DocID tiebreak never ran. Tallied by hop and summed in hop order, which
is what pkg/fusion's rank-major sweep already does one level up.
eval/s2: an all-zero embedding counted as coverage. Unlike a non-finite one it
is not refused anywhere — engine.Add stores it, build counts it, and
pkg/scorer/vector then skips the document for having no direction. A cache
full of them publishes a text+vector arm over a corpus its vector stream never
scores, which reads as a weak scorer rather than as absent data. Rejected with
the other unusable vectors.
eval/build: the vector width came from whichever vector the corpus order
presented first. One malformed response ahead of an otherwise uniform corpus
made the outlier authoritative and skipped every correct vector behind it —
and the build still committed, so the run went on to publish a vector arm
backed by the outlier alone. Decided over the whole cache now, with the
minority width named rather than only counted.
Each fix has the regression that fails without it; all four were verified
against the unfixed code.
Note for whoever reruns the measurement: the graph and s2 changes can move a
published number. Summing by hop changes the last bit of a graph score, and
more exact ties mean more DocID tiebreaking; rejecting zero vectors lowers
vector coverage if the cache holds any. Neither was re-measured here.
Six findings, one shape. In each, something ran a second time — a reference counted again, a failed read attempted again, a rejected request retried, a vector held in a second copy, a library reimplemented and never compared, a golden re-sorted between runs — and the thing that would have caught it was counting to one. eval/build: a reference could become two edges. A references list can name the same CorpusId twice, and duplicate-paper merging on the Semantic Scholar side can resolve one back to the citing document itself. Appended blindly, both land in Document.Links and both increment the edge total build prints. The traversal sees neither — it dedupes on visit, and a self-edge leads nowhere — so the surplus is a density claim about a graph the arms do not walk, which is the one kind of claim no ranking can corroborate or contradict. Deduped now, with the collapsed references counted and logged separately from dangling ones: those point outside the corpus, these point somewhere the traversal reaches anyway. eval/build: every SPECTER vector was alive twice. Nothing reads recs after corpusIDIndex and dominantDim have run, and Add clones the vector into the index, so the cache entry was a second copy of about 525 MB across the 171K document build, held for a map nobody looks at again. Released as consumed. eval/dataset: one unreadable row could spin forever. csv.Reader has consumed the line a *csv.ParseError refers to, so skipping it makes progress. A read error consumed nothing and comes back identically on the next call — counted as a skippable row, it loops without end over a 1.6 GB file, silently, in a command already measured in hours. Only parse errors are skipped now; everything else fails the read. eval/s2: a 400 whose body did not arrive was retried to exhaustion. do reads the body even on an error status, because that is where the endpoint puts its reason, and on a connection the server is already tearing down that read fails. Matching err ahead of status sent an unanswerable request down the retry path and spent every one of MaxRetries+1 attempts, backing off up to two minutes each, against a rate limit shared with every other anonymous caller. Status is classified first now, and the failed read is reported alongside it rather than in place of it. eval/s2: Pace was a property of one goroutine. last was read and written without a lock, so concurrent callers all measured their gap from the same stale timestamp and all decided at once that they had waited long enough. The lock is held across the sleep, not only around the assignment — that is what makes callers queue and leave one per Pace, which is what the field claims to mean. Documented on the type: Batch is safe from several goroutines, and parallelising changes how many requests are in flight, not how fast they go out. cmd/weft-eval: the rrf copy had never been compared to what it copies. It exists because sweep must vary a constant fusion.RRFk fixes, and docs/EVAL.md section 2.1 cites the sweep as what injecting the Fuser bought — but a change to the library's accumulation would have left the sweep measuring a function the library no longer has, with every cell of the 5.10 table coming from code nobody checked. TestRRFAtTheLibraryConstantIsFuse asserts bit equality at RRFk, including a case whose rank multisets are equal under a different per-stream order so a stream-major copy fails it, and asserts rrf(1) differs so a copy that ignores its parameter cannot pass by printing 28 identical cells. pkg/engine: the public API golden could reorder itself between runs. sort.Slice on the first line was justified by "two declarations cannot share one", which buildTag makes false: two files each excluded from every apiContexts entry both carry the tag " []" and may legally declare the same symbol. Stable now, so those land in ReadDir order — a golden that records a platform nobody builds is better than one that rewrites itself. Two comments corrected where they said more than the code does. FuseWeighted(2, 1) and FuseWeighted(1, 0.5) are the same Fuser over two streams only; over three the scaled implicit weight makes them 1:0.5:0.5 and 1:0.5:1, which the next paragraph already explains. And ReadCORD19IDs' "first row wins" is first row that yielded an identifier — a row with every join column empty stores nothing and holds no place against a later row for the same uid. go build, go vet and go test ./... all pass. Also gitignores __pycache__ and drops the .pyc that had been committed under testdata. Note for whoever reruns the measurement: the edge dedup can move a published number. 579,720 in-corpus edges appears in docs/EVAL.md, docs/FINDINGS.md and README.md, and it will fall by however many duplicate and self references the 2022-06-02 snapshot actually holds. That was not rebuilt here, so the figure is unverified rather than known-wrong — the build now prints the collapsed count, which is what the rerun should read. Nothing else in this round touches a ranking.
…o the one asked PR #7 review, six findings. The shape they share is narrower than the earlier rounds': in each one a check exists, runs, and passes — on a quantity that is adjacent to the one the rule names. The evidence was about another corpus, the sign came from the interval instead of the delta, the pinned files were not the input being pinned. eval/prepare: the evidence that a join can work was counted over the whole cache. It is the guard that separates "every joinable document is already cached" from "this metadata release does not match this corpus", and its evidence is a cached record carrying a CorpusId — counted without asking whose corpus the record's key belongs to. An s2.jsonl kept from an earlier dataset supplies it: none of the current keys match the metadata, the guard sees a working join and stands down, and every current document is written as a tombstone. build reads that as complete coverage and publishes vector and graph arms over an index with neither, which is the docs/EVAL.md section 4.1 failure rebuilt out of the check meant to stop it. The tally is now scoped to the corpus being prepared; keys, models and the resume offset still describe the file. eval: the corpus-pairing check skipped the nonrelevant judgments. idealDCG drops everything at or below 0, so a missing grade-0 document moves no term of the sum — true of the arithmetic, false of the ranking. A judged-nonrelevant document is one an assessor saw because a system retrieved it, so in the corpus those judgments belong to it takes a slot above the cut that something else takes here. It is also the same evidence of a mispaired qrels file and index as a missing relevant one. Measured before tightening rather than assumed safe: all 66,336 of trec-covid's judgments, including the 41,663 at grade 0, name documents in the BEIR corpus, so the stricter check refuses no run that was measuring correctly. eval-cmd: run, sweep and weights verified the files they read and not the index they measure. queries.jsonl and qrels/test.tsv are pinned; the index is a third input and nothing described it. `build -any-snapshot` against another corpus revision, followed by `run` beside the pinned judged inputs, therefore passed every check there was — and a revision keeping the same document keys satisfies the qrels check too, so different text, vectors and links get printed under the published labels. build now writes index/provenance.json recording the sha256 of the corpus it read, hashed at build time whether or not the snapshot check ran, and whether -partial was used; run, sweep, weights and diagnose refuse an index that does not match, cannot say, or was built over a cache not covering the corpus. -any-snapshot opts out, as everywhere else. eval-cmd: sweep took rule 2's sign from the confidence interval. Section 4 rule 2 is the sign of the observed delta and nothing else; the interval is rule 1's criterion at the frozen point. Reading them off one value made a cell whose interval spanned zero signless, and the flip count skipped it — so a grid going +0.02, -0.02, +0.02 under wide intervals established no first sign, counted no flip, and printed "rule 2 holds" about deltas that had flipped twice. The sign now comes from the delta, the delta gets its own column, and cells whose interval spans zero are counted and reported beside the verdict instead of being folded into it. eval: ReadCorpus streamed a duplicate _id without noticing. ReadQueries has refused a repeated query id all along, and this is the same fault at the more expensive end: prepare puts the key in its fetch list twice, asks Semantic Scholar about it twice, and writes two records under one key — which build rejects, after the hours of rate-limited fetching that produced them. The ids are now held and checked; a set of 171K keys is a few megabytes beside the documents the streaming exists to avoid holding. eval-cmd: -iters=0 was rejected after every arm had been evaluated. BootstrapCI refuses it correctly and is reached last, so on the documented corpus that is 50 queries against 171,332 documents with a brute-force vector scan each, spent to arrive at a flag error decidable before the index was opened; sweep is worse, at three arms per grid cell. Checked immediately after parsing in all three commands, with an empty -data directory in the test so a check that drifts back below the index open fails on a missing file instead. Each fix has the regression that fails without it. make all 7/7, make arch 7/7 including the public API golden, make deps unchanged. The published numbers are being re-measured in the same working tree and will follow in their own commit: the previous round's duplicate-reference dedup collapsed exactly one edge, 579,720 to 579,719, and that moved the binding delta from -0.1202 to -0.1227. Nothing in this round touches a ranking.
…ify them PR #7 review, second pass, three findings. Each accepts a value on the strength of something that looks like identity and is not: a vector's width, a flag's sign, the ratio between two weights after rounding. fusion: scaling the weights could reorder the fusion it was documented not to reorder. scaleDown divides every weight by the largest when that exceeds 1, on the argument that only the ratios affect the fused order. True of the arithmetic and not of float64: the division rounds, and it rounds a document's one term differently from another document's two. At weights 3, 2, 1 and RRFk=60 a document at rank 152 of the first stream outscores one at ranks 92 and 947 of the other two by one ulp; divide by 3 and the totals become equal, at which point TopK settles them on DocID — the collapse to insertion order every other guard in that file exists to prevent, reached through the one operation that promised to change nothing. Scaled by a power of two now, via Frexp and Ldexp, which adjusts the exponent and leaves the significand alone: every score comes out as the unscaled score over 2^exp to the last bit, so the order cannot move, and the overflow bound the scaling exists for is slightly tighter than before (maxW lands in [0.5, 1)). No weight in this repository exceeds 1, so scaleDown does not fire and no published figure moves. eval/build: a vector was indexed because its width matched. SPECTER v1 and v2 both emit 768 dimensions, so dominantDim accepts either and engine.Add stores either, while the query vectors come from gen_query_vectors.py under eval.S2Model. Cosine similarity across two embedding spaces is not a similarity, and it arrives as a plausible vector baseline with the whole graph delta measured against it. Nothing downstream could catch it: a committed index carries no model label, and prepare's warning belongs to whichever invocation fetched the batch, which on a resumed job is not the one that finishes. build now tallies the models behind the corpus's cached vectors and refuses a foreign one, with -partial as the override — the same flag that already means "these arms are not publishable", and which run, sweep and weights refuse by the index's provenance. An unrecorded model is warned about and indexed: that is what the committed measurement was fetched into, all 148,232 of its vectors, so refusing it would refuse the published index rather than a mistake. eval/prepare: -limit=-1 meant unlimited. 0 is the documented unlimited value and the guard read `limit > 0`, so a smoke-test typo started fetching the entire corpus — hours of rate-limited requests against a shared anonymous budget, appending to the resumable cache the whole way, with nothing in the log saying the flag had been ignored. Rejected before the corpus is read and before anything is written. Each fix has the regression that fails without it; the fusion one was run against the unfixed scaleDown and reports the tie it produces. make all 7/7, make arch 7/7 including the public API golden, make deps unchanged.
The note the round 7 commit left for whoever reran the measurement. This is that rerun, against the real .eval-data snapshot, and the answer is that the dedup was worth exactly one edge and 0.0025 nDCG. **579,720 in-corpus edges becomes 579,719.** One reference in the whole corpus either named a CorpusId its document already linked or resolved back to the document itself. The traversal walked neither, which is why the count was the only thing they inflated. **The binding delta went from -0.1202 to -0.1227**, interval [-0.1550, -0.0909]. One edge in 579,720 moving the headline by 0.0025 is not noise and not a flaw in the fix: it is section 5.9's degeneracy seen from the other side. The modal graph score is 0.5, the tie group crosses the cut on 41 of the 45 queries that answer at all, and 960 slots are settled by DocID — so a single changed adjacency re-decides a whole tie group, and the tie group is most of what the graph stream contributes. EVAL 5.13 says this next to the number rather than leaving a reader to infer it. Every current figure retaken with `make eval-full` against the rebuilt index: - arms: text+graph 0.3987 to 0.3985, text+vector+graph 0.5031 to 0.5005, text+vector+graph-including-seeds 0.5464 to 0.5451 - comparisons: binding above; text+graph - text -0.1839 to -0.1841; the seeds control -0.0769 to -0.0782 - tie analysis: 958 slots decided by DocID to 960, distinct scores per query 3-29 to 3-24; still 41 of 45 queries, still 3 as the mode - sweep: 28 configurations, 0 sign flips on both pairs, unchanged. Worst-case binding CI upper bound still -0.0065. The comparison column's worst case moved from -0.1261 to -0.1278 and its delta range from -0.1762..-0.1895 to -0.1786..-0.1896 - weights: unchanged where it matters — best available delta still exactly +0.0000, still converging onto the baseline by weight 0.1 text at 0.5826 and text+vector at 0.6233 are identical to four decimals, as they must be: neither fuses a graph stream. That is the control on the rebuild. The verdict does not move. Section 4 rule 1 still fails in the negative direction with the interval well clear of zero, rule 2 still holds with 0 flips, and no fusion weight makes the graph stream worth anything. Sections 5.8 and 5.12 keep the numbers measured at the time; they describe states this repository has left behind. EVAL 5.13 is new and also records what the round 8 review added to the pipeline: an index now carries index/provenance.json naming the corpus it was built from, so the arms cannot be published over one nobody can identify. Updated in EVAL.md, FINDINGS.md, DECISIONS.md and README.md. make all 7/7, make arch 7/7, make deps unchanged.
…bes could disagree PR #7 review, third pass. Three of these are provenance — a record that could outlive what it describes, an artifact that recorded nothing, a flag parse that discarded what it had been given — and the fourth is a sentence claiming more than the grid under it measured. eval/build: the provenance record could outlive the index it describes. A rebuild replaced the segments and then wrote provenance.json, and a crash in that window left the previous record standing beside a manifest it does not describe. A later run verifies the stale record and accepts a foreign or partial index as the pinned one — the substitution provenance exists to refuse, reached through provenance itself. The old record is now removed before the commit, so an unfinished rebuild leaves an index that cannot say what it holds, which is the answer verifyProvenance already refuses. The regression fails a commit on a corrupt manifest and asserts nothing describes the index afterwards. eval: the query vectors recorded no model, while the document side had just started refusing one. Same hazard from the other end — a file generated from another adapter, base revision or local model configuration carries the right query id and the right question text, so the id check and the text check both pass and the vector scorer computes cosine similarity between two embedding spaces. gen_query_vectors.py now writes the model into every record, and loadQueries refuses one that names anything but the SPECTER2 base plus the ad-hoc query adapter. Unrecorded is tolerated on both sides, for the same reason: the committed query-vectors.jsonl and all 148,232 committed document vectors predate their field, so refusing absence would refuse the published measurement rather than a mistake. eval-cmd: a positional typo silently discarded every flag after it. flag stops parsing at the first non-flag argument and reports nothing, so `weft-eval prepare typo -limit=1` runs with -limit at its default — and the default means unlimited, which is hours of rate-limited API requests appending to the resumable cache with the flag meant to keep it small thrown away. dataFlags now refuses a leftover argument; no subcommand takes one. docs: "no weight beats the baseline" was a claim about eight sampled weights. nDCG is not monotonic in the fusion weight — a weighted RRF ranking changes at query-specific score-crossing thresholds — so the ends of an unsampled interval do not bound what happens inside it. Split into the two claims that were being run together: at 0.1 and below the arm is bit-identical to the baseline, so every weight in that region provably changes nothing; above it, the honest statement is that no *tested* weight beats the baseline, with 0.25-to-0.1 named as where an untested value would have to hide. EVAL 5.11 states it in full and says why it is a thin hope rather than an open question; README, FINDINGS and the verdict table now say "in the tested grid". No number moves — this is what the numbers were already entitled to mean. Each fix has the regression that fails without it. make all 7/7, make arch 7/7 including the public API golden, make deps unchanged. No published figure changes.
…not been re-measured PR #7 review, fourth pass, two findings. eval-cmd: judgments for a query the query file does not hold were unreachable. loadQueries walks the queries and looks each one's judgments up, so a qrels row naming a query absent from queries.jsonl is not skipped, not counted and not reported — it is never reached. A truncated or mismatched query file therefore yields a mean and a bootstrap over however many queries survived, printed under the usual heading with a query count nobody compares to 50. Dropping a judgment-less query is the deliberate case and is already counted; this is its mirror image and there is no reading of it that is not a broken pairing, so it is refused, with the offending id named in sorted order. Checked even under -any-snapshot: that flag says "a different corpus", not "a query set and its judgments that disagree". Measured before tightening — all 50 of trec-covid's qrels query ids are in queries.jsonl and all 50 queries are judged — so it refuses no run that was measuring correctly. docs: the exported Go documentation still published the pre-re-measurement numbers. pkg/scorer/graph's package doc is where a user of the library meets this verdict, and it named 579,720 edges and −0.1202; pkg/fusion/rrf.go repeated −0.1202 twice as the motivation for FuseWeighted, and weights' own doc comment carried it as the number it exists to explain. Someone reading `go doc` saw evidence disagreeing with the repository's published result. Updated to 579,719 and −0.1227 alongside README.md and docs/EVAL.md; the +0.0000 best case and the 0.0019 recovered by halving the weight are unchanged, so no sentence around them needed rewriting. make all 7/7, make arch 7/7, make deps unchanged. `weft-eval run` against the committed index still reproduces every figure in docs/EVAL.md section 5.9 with all of this round's and the previous rounds' checks in force — including the provenance gate, which the rebuilt index passes.
PR #7 review, fifth pass, two findings. One is a published number that was describing something other than what it was labelled; the other is a property worth pinning rather than a regression. eval/diagnose: "slots decided by DocID" counted the candidates that lost, not the slots they lost. A tie group straddling the cut was tallied by how many cut-score candidates fell *outside* the top k, and the summary called that total "slots decided by DocID". The two come apart badly: 100 candidates tied for 10 positions is 90 excluded and 10 arbitrary slots, and reporting 90 makes a claim about a top-10 that a top-10 cannot hold. The published figure was of that kind — 960 across 45 rankings of 10. Both counts are now measured and printed, because they answer different questions: how large the tie group was, and how much of the reported answer it decided. On the current index, 41 of 45 queries have a tie group crossing the cut, **241 of the reported slots are held at the cut score, and 960 further candidates are excluded from it by DocID alone.** docs/EVAL.md section 5.9's table row is relabelled rather than corrected — 2,082 and 960 were always the excluded count, in both the before and after columns, so they are comparable and stay. The slots figure is a new row, with "not measured" for the pre-fix column: it would take reverting the graph scorer to obtain, and nothing in the argument needs it. Section 5.13 and FINDINGS §5 now say both numbers where they used to say one. fusion: the underflow at the far end of the weight range is documented and pinned, not changed. The report reads as a regression from the power-of-two scaling of round 9 and is not one — 1e-300 against math.MaxFloat64 underflows to zero identically under `w /= maxW` and under `Ldexp(w, -exp)`, because the ratio is past what float64 holds either way. What the report is right about is the consequence, which the comment had glossed: weight 0 means fuse does not create the entry, so the stream's documents are absent from the result rather than last in it. Stated exactly now, with the reason the trade goes this way — overflow takes the ranking apart for every stream at once, underflow costs the one stream that asked to be 1e308 times quieter than another — and TestScaleDownDropsAStreamItCannotRepresent pins both sides, including that a representable ratio still ranks the quiet stream rather than dropping it. make all 7/7, make arch 7/7, make deps unchanged. No arm number, interval or sign moves: diagnose reports on the graph stream before fusion and is not an input to any of them.
PR #7 review, sixth pass, two findings. Both are about a guard that was satisfied by something weaker than the thing it was guarding. fusion: a document's weighted votes were summed in stream order. Rank-major accumulation makes a total a function of the ranks a document earned rather than of the order the scorers were listed in, and within one rank that used to rest on "repeats of a rank add the same value and commute" — true unweighted, false with weights. Three streams meeting at rank 1 with weights 0.25, 1/6 and 0.125 sum to one of two different last bits depending on the order they are visited, and against a competitor weighted 0.5416666666666666 that bit is the difference between winning outright and tying and losing on DocID. Moving a scorer and its weight together is the same fusion and could produce a different ranking. Streams are now visited lightest weight first within each rank, so the sum is a function of the multiset of votes a document actually earned. The sort is stable, so equal weights keep slice order and the unweighted path — every ranking milestones 1 and 2 pin — is bit-identical. TestFuseWeightedIsInvariantToStreamOrder enumerates all 24 permutations of the four (stream, weight) pairs above and was run against the previous accumulation, where it fails. The weight sweep was re-measured because it is the one published table that fuses with weights: every cell is identical, including the -0.0019 at 0.5 and the exact +0.0000 from 0.1 down. docs/EVAL.md section 5.11 needs no edit. testdata: the query-vector sanity check passed on a bare majority of wins. It compares each query's mean cosine to its judged-relevant documents against its mean cosine to random ones, and accepted any result with more wins than losses — which for 50 queries is 26, the median outcome of a coin flip. An adapter embedding into the wrong space produces exactly that, so the check would have admitted the case it exists to refuse, and the aggregate cosines printed immediately above it were never compared at all. It now requires a one-sided sign test at p < 0.001, computed exactly with math.comb so the script stays on the standard library, and a positive aggregate margin. The recorded run — 50/50 wins, p = 8.9e-16, means 0.7620 against 0.6842 — clears both by a wide margin; 34 of 50 wins would fail. docs/EVAL.md section 5.4 records the p-value and why the bar moved. make all 7/7, make arch 7/7, make deps unchanged. No published figure moves.
Six unresolved review threads, and the governance gate main just grew. `prepare -any-snapshot` said the numbers were not publishable and then let a plain `build` say they were: the cache it leaves is an ordinary complete one, so coverage passes, the corpus hash matches, and partial=false goes into the record. The claim is now a marker beside the cache, carried into provenance.json as prepare_unpinned and refused by run, sweep, weights and diagnose. The rest, each where it belonged: - `Evaluate` accepts an arbitrary Fuser and scored whatever it returned. A repeated DocID credits one grade at two ranks against an ideal that counts it once, which is nDCG above 1.0 — now ErrDuplicateRanked. - The 1.6 GB metadata scan takes no context, so a Ctrl-C during it was absorbed and the tombstones were appended anyway. Checked before the cache is touched. - The query-adapter sanity check drew its random pool from the whole cache, which `build` allows to hold another corpus. Restricted to the current one, as --verify already was. - `make eval-data` prepared and built without the query vectors, so `make eval` succeeded with the vector scorer abstaining and arms labelled text+vector that were text-only. It now refuses to call preparation complete. - FINDINGS §7 said the weighted variant lived in cmd/weft-eval and that FuseWeighted shipped. Both were true, in that order; it now says so. Rebased onto main's governance work, which turned CI into a gate this branch had never been judged by: SPDX headers on 15 files, and 24 golangci-lint findings. Nine were real and are fixed. The rest are exclusions, each naming what makes the finding wrong here — the eval commands are branchy because almost every branch is a refusal, and the bootstrap needs a seeded PRNG because the published intervals have to be re-derivable. 156 markdownlint errors, all table and code-fence style. Re-measured: 2,753 implementation lines under pkg/, 5,138 test.
ae368fa to
39b2db3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39b2db372f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // NaN sorts nowhere in particular and is dropped by the guard below without | ||
| // reaching the sum, so where it lands cannot matter. | ||
| sort.SliceStable(order, func(a, b int) bool { return weightOf(order[a]) < weightOf(order[b]) }) |
There was a problem hiding this comment.
Normalize NaN weights before sorting
When a NaN weight lies between two valid weights, this comparator treats it as equivalent to both and therefore does not provide a strict weak ordering; for example, stable-sorting weights [2, NaN, 1] can leave them in that order. Although the NaN stream is skipped later, it can prevent the valid streams from being ordered canonically, so their floating-point contributions are again accumulated in caller order and reordering scorer/weight pairs can change a last bit and the final ranking. Convert disabled or invalid weights to a sortable sentinel, or remove those streams from order before sorting.
Useful? React with 👍 / 👎.
| if anySnapshot { | ||
| // Written before the first append, so a run killed halfway still leaves the | ||
| // cache described. See s2UnpinnedFile. | ||
| if err := markUnpinned(unpinnedPath); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Delay marking the cache until it is modified
When prepare -any-snapshot is run against an already complete pinned cache, this creates s2.unpinned before the cache scan discovers that there is nothing to fetch and returns without changing s2.jsonl. A subsequent ordinary build nevertheless records PrepareUnpinned=true, making a previously publishable index permanently fail provenance verification; failed invocations before the first append leave the same false taint. Create the marker immediately before the first actual cache write instead.
Useful? React with 👍 / 👎.
| eval-data: | ||
| go run ./cmd/weft-eval prepare | ||
| @if [ ! -f $(EVAL_DATA)/query-vectors.jsonl ]; then \ |
There was a problem hiding this comment.
Pass EVAL_DATA to the evaluation commands
When a caller overrides the newly exposed EVAL_DATA variable, this recipe still runs prepare and build with their default .eval-data directory while the intervening check examines $(EVAL_DATA). Thus make eval-data EVAL_DATA=/some/path can spend hours fetching into the wrong cache, validate a query-vector file from another directory, and build the default index instead of the requested one. Pass -data $(EVAL_DATA) consistently to both subcommands.
Useful? React with 👍 / 👎.
Pull Request
Summary
Milestone 4 exists to answer the PRD's second falsification condition: does graph proximity improve ranking quality? Everything before this milestone measured a property of our own code — fusion is invariant to scorer count, a reopened index ranks identically — and those cannot be argued with. This one measures a claim about the world, so the dangerous failure was never a bug, it was believing a wrong number.
The answer is no. On TREC-COVID joined to the Semantic Scholar citation graph, adding the graph scorer costs 0.1156 nDCG@10 (95% CI [−0.1483, −0.0837]) against the pre-registered
text+vectorbaseline, and the sign does not flip across 28 configurations of the RRF rank constant and fusion depth.The larger finding is about fusion, not about graphs. That −0.1156 belonged to RRF's equal vote. Halving the graph stream's weight erases the entire regression, and at the best weight the signal is worth +0.0018 with an interval touching zero. So the accurate statement is the flatter one: the graph signal is not harmful information, it is not information — and the biggest measured effect in the milestone sits in the fusion operator, which is now addressable via
fusion.FuseWeighted.What changes for a user of the library:
pkg/fusiongains one function,pkg/scorer/graphscores differently and carries the verdict at the top of its documentation, and nothing else in the public API moves.Full measurement design, coverage and reasons to doubt the numbers:
docs/EVAL.md. Verdict:docs/FINDINGS.mdmilestone 4. Decisions: D-004, D-005.Changes
internal/eval— nDCG@10, an arm runner, a paired bootstrap and the dataset readers, standard library only. The harness never learns what a graph scorer is: anArmis a name, a[]engine.Scorer, aFuserand a depth, and five arms differ only in the contents of a slice.trec_evaluses linear gain, not the exponential form the plan specified — which would have put every figure on a scale nobody else uses. BM25 agrees withrank_bm25to 4.44e-16, closing the PRD's long-unclaimed "correctness floor" row.fusion.FuseWeighted— per-stream weights indexed by position, not scorer kind. The caller already fixed that order, so fusion still learns nothing about what it holds.Fuseis unchanged and its unweighted path is bit-identical.scorer/graphto sum per-seed distances rather than take the nearest, the fix the plan named in advance if the degeneracy diagnostic bit. It bit — 38 of 50 queries had their top-k decided byDocID, i.e. insertion order — and the fix recovered +0.044 of the 0.12 needed. Both measurements are kept.ponytail:marker atsearch.go:112rather than repaying it.Fusescores from ranks alone, soSearch(ctx, q, k*m, ...)truncated tokequals fusing tok— the ceiling it named was reachable from outside all along.cmd/weft-eval(prepare/build/diagnose/run/sweep/weights) andmake eval/eval-full/eval-data, so every published figure re-derives from one command.Validation
make all— build, vet,go test -race ./..., 7/7 packagesmake arch— 7/7, milestone 1 assertions unchanged, including the golden engine API filemake deps— one module, andgo list -deps ./pkg/fusionstill names no scorer packagemake eval— reproduces every figure indocs/EVAL.mdfrom a committed index; bootstrap seed and frozen constants are compiled in, so the intervals are the published ones rather than approximationsweft-eval build— reopened index matches the committed one on document count and average length, the first real-corpus exercise of milestone 2's restore equivalence (171,332 documents, commit 1.9 s, reopen 963 ms)internal/eval/testdata/, so the two comparisons above can be re-derived rather than taken on trustReview Focus
pkg/fusion/rrf.go—Fusenow delegates to a shared loop. The claim that the unweighted path is bit-identical is what every ranking pinned by milestones 1 and 2 rests on;TestFuseWeightedWithoutWeightsIsFuseasserts it exactly rather than approximately.FuseWeighted's handling of out-of-contract weights. Negative, NaN and −Inf all fold onto the weight-0 behaviour via!(sw > 0). NaN is the one that matters: left alone it would make every score it touches NaN, andTopKwould quietly return corpus insertion order.pkg/scorer/graphseed handling. The score is now a sum, so a seed can be reached by a sibling seed and is no longer identifiable byhops == 0; exclusion is by identity, and duplicate seed keys are deduplicated before traversal.docs/EVAL.mdsection 4.1 — a finding this milestone published to itself and withdrew. Worth reading as a check on whether the correction is stated plainly enough.Risks / Notes
Fuse,Search,Scorerand the on-disk format are untouched; the engine's golden API file is unchanged.pkg/fusiongainsFuseWeighted.scorer/graphscores differently. Single-seed results are unchanged by construction, but a multi-seed query now ranks documents several seeds agree on above documents only one seed reaches. Anyone depending on the old nearest-seed values should read the package documentation.FuseWeightedacquiring no caller outsideinternal/evalwhilescorer/graphstays unweighted at milestone 6.docs/EVAL.mdsection 5.10 rather than left for a reader to notice.FuseWeighted's documentation says a caller without its own measurement should useFuse..eval-data/is gitignored, ~3 GB).docs/EVAL.mdsection 7 lists the downloads and the one-timepreparestep; it is rate limited and takes about 1h35m, and it is resumable.