Skip to content

Fix MedLink BM25 hard-negative mining - #1195

Open
fbonc wants to merge 5 commits into
sunlabuiuc:masterfrom
fbonc:medlink-fix
Open

Fix MedLink BM25 hard-negative mining#1195
fbonc wants to merge 5 commits into
sunlabuiuc:masterfrom
fbonc:medlink-fix

Conversation

@fbonc

@fbonc fbonc commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Problem
get_bm25_hard_negatives scored the corpus against the positive document (bm25_model.get_scores(d)) instead of the query (q). Hard negatives should be documents that the query ranks highly but that are not correct matches.

Negative selection also excluded only the current positive (neg_d_id != d_id). Once scoring uses the query, another valid match can rank near the top and be incorrectly selected as a negative when a query has multiple positives, which record linkage allows.

After preserving all positives, the resulting qrels could contain multiple positives and one negative. get_train_dataloader still asserted that each query had at most two entries and stored only one positive, causing valid multi-positive qrels to crash before training.

Fix
Score documents against the query and exclude every positive document from negative selection.

Preserve all positives in the mined qrels. During dataloader construction, create one training sample per positive and reuse the query’s hard negative:

(query, positive_1, negative)
(query, positive_2, negative)

Single-positive queries and queries without a hard negative retain their existing behavior.

Tests
Added controlled fake-BM25 regression coverage verifying that:

  • The negative comes from scoring the query, not the positive document.
  • No positive is selected as a negative when a query has multiple matches.
  • All positives remain after mining and are emitted as separate training samples by the dataloader.

@lehendo

lehendo commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

qrels_w_neg[q_id] = {d_id: 1, neg_d_id: -1} on line 140 ish I think, overwrites the query's entry each loop iteration instead of merging. So if you see for queries with 2+ positives, then only the last one survives. earlier positives are silently dropped from the output, not just correctly excluded as negatives. Also, test_hard_negatives_exclude_all_positives only checks positives aren't labeled -1, not that they're still present. the test doesn't catch this.

@fbonc

fbonc commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

qrels_w_neg[q_id] = {d_id: 1, neg_d_id: -1} on line 140 ish I think, overwrites the query's entry each loop iteration instead of merging. So if you see for queries with 2+ positives, then only the last one survives. earlier positives are silently dropped from the output, not just correctly excluded as negatives. Also, test_hard_negatives_exclude_all_positives only checks positives aren't labeled -1, not that they're still present. the test doesn't catch this.

fixed

@DarylOkeke

Copy link
Copy Markdown

This now keeps every positive, but get_train_dataloader() still allows at most two qrel entries. Two positives plus the new negative makes three, so the new multi-positive case crashes when passed to the loader. Could we either keep one positive or update the loader and add an end-to-end test?

@fbonc

fbonc commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author

This now keeps every positive, but get_train_dataloader() still allows at most two qrel entries. Two positives plus the new negative makes three, so the new multi-positive case crashes when passed to the loader. Could we either keep one positive or update the loader and add an end-to-end test?

Already fixed in previous commits I believe

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.

3 participants