[fix] Make relative margin sign-independent in mining and GIST losses#3821
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a sign-dependent bug in “relative margin” filtering for hard negative mining and GIST losses by switching from positive_score * (1 - margin) to an order-preserving threshold positive_score - abs(positive_score) * margin when deciding which negatives to suppress. This keeps behavior unchanged for positive scores while correctly filtering “too-close” negatives when the positive score is negative.
Changes:
- Update
mine_hard_negatives(relative_margin=...)to use a sign-independent relative threshold. - Update
GISTEmbedLossandCachedGISTEmbedLossrelative-margin masking to usepositive - abs(positive) * margin. - Add deterministic CPU-only regression tests covering negative positive-pair scores for all three code paths.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/util/test_hard_negatives.py | Adds a controlled-model regression test for negative positive-pair scores in mine_hard_negatives. |
| tests/sentence_transformer/losses/test_gist_embed.py | Adds a regression test ensuring relative-margin masking suppresses closer negatives when the positive score is negative. |
| tests/sentence_transformer/losses/test_cached_gist_embed.py | Adds the analogous regression test for CachedGISTEmbedLoss. |
| sentence_transformers/util/hard_negatives.py | Implements the sign-independent relative threshold and updates user-facing docs/example text. |
| sentence_transformers/sentence_transformer/losses/gist_embed.py | Updates relative-margin masking logic and docstring formula for GISTEmbedLoss. |
| sentence_transformers/sentence_transformer/losses/cached_gist_embed.py | Updates relative-margin masking logic and docstring formula for CachedGISTEmbedLoss. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hello! Thanks a lot for this, and for reporting the issue in the first place! I appreciate that you also caught the same sign-dependent expression in the I went through it carefully: the new threshold matches the old I've pushed a few small doc follow-ups (the training overview and skill reference still had the old wording, plus some docstring tightening from Copilot's review). Merging once the tests go green, thank you for the contribution!
|
|
Thanks @tomaarsen! Really glad this was useful, and thanks for the doc follow-ups and the merge. |
Summary
Fixes #3819.
The relative margin used to discard negatives too similar to the anchor was computed as
positive_score * (1 - margin). That works for positive scores, but it is sign-dependent: when the positive-pair score is negative, the threshold moves above the positive score. For example, with a positive-pair score of-0.50andmargin=0.05, the threshold becomes-0.475, so a negative scoring-0.49survives even though it is more similar than the positive.This PR applies a score-order threshold,
positive_score - abs(positive_score) * margin, in the three places that shared the old expression:mine_hard_negatives(therelative_marginargument)GISTEmbedLoss(margin_strategy="relative")CachedGISTEmbedLoss(margin_strategy="relative")It is identical to
positive_score * (1 - margin)for positive scores (existing behavior unchanged) and keeps the threshold below the positive score when it is negative.Tests
Added deterministic, CPU-only regression tests for the negative-score case in all three. They fail on current
mainand pass with this change: