Repository navigation
Conversation
In ClapModel's return_loss path, the audio term transposed logits_per_audio, whose rows are already the audio-to-text direction. The loss therefore averaged the text-to-audio term at both logit scales and never included the audio-to-text term. Drop the transpose and add a regression test that builds the model with distinct logit scales, checks the two directional terms actually differ, and asserts the returned loss equals their average. Fixes huggingface#50227
Contributor
|
Thank you for your contribution 🤗! CI Security Gate — waiting for maintainer approvalCI is approved automatically only for authors who already have a contribution merged in this repository. A maintainer will review this PR and approve CI to run on it. |
Contributor
|
[For maintainers] Suggested jobs to run (before merge) run-slow: clap |
Member
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
ClapModel.forwardwithreturn_loss=Truecomputes the audio-side term ascontrastive_loss(logits_per_audio.t()). The rows oflogits_per_audioare already the audio→text direction, so the transpose turns that term back into a second text→audio cross-entropy — at the audio logit scale — and the returned loss averages the text→audio term at both scales. The audio→text term is never computed: that direction gets no training signal from the loss, andlogit_scale_areceives gradient only through the wrong-direction term.This PR drops the stray
.t(), so the loss is the symmetric average of one text→audio term and one audio→text term, each at its own scale. Inference is unaffected — the changed block only runs underreturn_loss=True, and the returned logits and embeddings are bit-identical before and after the change.For context, the transpose dates to #21670, where
contrastive_loss(logits_per_text.t())was changed tocontrastive_loss(logits_per_audio.t()). The transpose made sense for the text matrix; it looks like it was carried over when the argument was swapped to the audio matrix.Reported in #50227 — thanks to @Andreas-Hum for the precise report. He originally offered to open the fix PR himself; I coordinated on the issue first, and I am opening this PR at his invitation: #50227 (comment)
Tests: the new regression test
ClapModelTest::test_loss_is_symmetricbuilds a smallClapModelwith distinct logit scales (as in the released checkpoints), asserts the two directional terms actually differ (so the check is not vacuous), and asserts the returned loss equals their average. It fails on the unfixed code and passes with this fix. The fulltests/models/clap/test_modeling_clap.pyfile passes: 224 passed, 363 skipped.Fixes #50227
Code Agent Policy
I am not checking the box above because it would not be accurate: as I already disclosed on the issue, I used an AI assistant for parts of this analysis. The reproduction, the fix, and the test were all run and verified locally, and I can walk through any of it line by line. @Andreas-Hum knew this from the issue thread when he invited the PR.
Before submitting
Pull Request checks?
to it if that's the case.
Who can review?
@eustlb @ebezzam — audio models, and already cc'd on the issue.