Skip to content

fix(magic): score per-query MAGIC by document, not by row - #414

Closed
luciaquirke wants to merge 1 commit into
mainfrom
fix/per-query-chunked-queries
Closed

fix(magic): score per-query MAGIC by document, not by row#414
luciaquirke wants to merge 1 commit into
mainfrom
fix/per-query-chunked-queries

Conversation

@luciaquirke

@luciaquirke luciaquirke commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Opus slop

The bug

Per-query MAGIC crashes on a chunked query set (--query.chunk_length 32, i.e. any raw dataset you let bergson chunk):

IndexError: index 2 is out of bounds for dimension 0 with size 2
  bergson/magic/data_stream.py:132   "example_weight": self.weights[indices]

A chunked query set carries a per-token doc_ids column: a row can pack several documents and a document can span several rows, so query i is not row i. compute_per_query_magic_scores selected row qi and sized the per-query stream's weights by row, while DataStream indexes weights by document id whenever the batch has doc_ids — hence the overrun. Unchunked query sets (every shipped example uses chunk_length: 0) have no doc_ids column and were unaffected.

Fix

Score per document, which is the unit everything downstream already uses (validate_scores requires one column per query document; per_doc_query_losses aggregates losses by doc_ids):

  • pick the rows holding document qi's tokens,
  • mask every other document's tokens out of the labels, so the loss is that document's own mean cross-entropy,
  • drop doc_ids from the per-query stream, so its row-sized weights are indexed by row. compute_query_gradients deletes example_weight anyway — the document restriction now lives in the labels.

Padding cycles through the document's own rows instead of appending dead ones: a pad row is a real term in the query loss (the weights that would silence it are discarded), and an all-pad batch on some rank would silently scale the query gradient by 1/world_size. With one row per document this repeats that row — exactly what pad_dataset_to_batch_size did — so unchunked runs are bit-for-bit unchanged.

Chunking drops the tail that doesn't fill a chunk, so a short document (e.g. a blank line in wikitext) can leave no tokens behind. Those score zeros, matching the zero baseline loss per_doc_query_losses gives them; their correlation is undefined, so the reported mean Spearman now averages the queries that have one rather than coming out nan for the whole run:

@luciaquirke
luciaquirke force-pushed the fix/per-query-chunked-queries branch 3 times, most recently from bbbc185 to 6c0edd3 Compare August 6, 2026 11:33
A chunked query set (`query.chunk_length > 0`) carries a per-token
`doc_ids` column: a row can pack several documents and a document can
span several rows. `compute_per_query_magic_scores` selected row `qi`
for query `qi` and sized the stream's weights by row, so `DataStream`
— which indexes the weights by document id whenever the batch has a
`doc_ids` column — walked off the end:

    IndexError: index 2 is out of bounds for dimension 0 with size 2
      bergson/magic/data_stream.py:132  self.weights[indices]

Repro: `bergson magic <run> --query.chunk_length 32 ...`.

Pick the rows holding each document's tokens instead, mask every other
document's tokens out of the labels, and drop `doc_ids` from the
per-query stream so the weights (which `compute_query_gradients`
discards anyway) are indexed by row. Each column is then that
document's own mean cross-entropy, which is the unit `validate_scores`
and `per_doc_query_losses` already score against.

Padding cycles through the document's own rows rather than appending
dead ones — a pad row is a real term in the query loss, and an all-pad
batch on some rank would silently scale the query gradient down. With
one row per document this repeats that row, exactly as
`pad_dataset_to_batch_size` did, so unchunked runs are unchanged.

Chunking drops the tail that doesn't fill a chunk, so a short document
can leave no tokens behind; those score zeros, matching the zero
baseline loss `per_doc_query_losses` gives them. Their correlation is
undefined, so the reported mean Spearman now averages the queries that
have one instead of coming out nan for the whole run.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant