Skip to content

Fix CLAP loss double-counting the text-to-audio direction - #50319

Open
0xABCD01 wants to merge 1 commit into
huggingface:mainfrom
0xABCD01:fix/clap-loss
Open

0xABCD01 wants to merge 1 commit into
huggingface:mainfrom
0xABCD01:fix/clap-loss

Conversation

@0xABCD01

@0xABCD01 0xABCD01 commented Nov 3, 2026 •

Copy link
Copy Markdown

🤗 Transformers CI

What does this PR do?

ClapModel.forward with return_loss=True computes the audio-side term as contrastive_loss(logits_per_audio.t()). The rows of logits_per_audio are 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, and logit_scale_a receives 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 under return_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 to contrastive_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_symmetric builds a small ClapModel with 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 full tests/models/clap/test_modeling_clap.py file passes: 224 passed, 363 skipped.

Fixes #50227

Code Agent Policy

  • (First-time contributors only): I confirm that this PR description and code is not written by an LLM or code agent

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

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline and the
    Pull Request checks?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes according to the guidelines?
  • Did you write any new necessary tests?

Who can review?

@eustlb @ebezzam — audio models, and already cc'd on the issue.

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
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Thank you for your contribution 🤗!

CI Security Gate — waiting for maintainer approval

CI 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.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

[For maintainers] Suggested jobs to run (before merge)

run-slow: clap

@Rocketknight1

Copy link
Copy Markdown
Member

cc @eustlb @ebezzam for audio

This branch has not been deployed

No deployments
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.

[CLAP] ClapModel(return_loss=True) computes the text-to-audio loss twice and never the audio-to-text loss

2 participants