Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CPO & SimPO Loss #749

Merged
merged 75 commits into from
Jul 22, 2024
Merged

CPO & SimPO Loss #749

merged 75 commits into from
Jul 22, 2024

Conversation

psinger
Copy link
Collaborator

@psinger psinger commented Jun 6, 2024

This PR implements new CPOLoss and SimPOLoss

Please review after #753

Closes #732

@psinger psinger marked this pull request as ready for review July 11, 2024 12:18
Copy link
Collaborator

@pascal-pfeiffer pascal-pfeiffer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the integration @psinger !

It seems to work nicely, though default values give bad results.
We might want to consider not to rename the Loss functions to avoid issues when starting from older experiments.
Also, there is a minor issue with the calculation of the number of experiments being started in grid search.

@psinger
Copy link
Collaborator Author

psinger commented Jul 19, 2024

It seems to work nicely, though default values give bad results.

Bad in what way? It is based on best experiments I got for them, but it might be more sensitive depending on what model / data one uses.

@pascal-pfeiffer
Copy link
Collaborator

It seems to work nicely, though default values give bad results.

Bad in what way? It is based on best experiments I got for them, but it might be more sensitive depending on what model / data one uses.

Just running the default experiment with Loss function changed to SPO gives for example these predictions:
image

But it is probably fine, it is expected that these things need tuning.

Copy link
Collaborator

@pascal-pfeiffer pascal-pfeiffer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @psinger , lgtm!

@psinger psinger merged commit 3aeab4a into main Jul 22, 2024
4 checks passed
@psinger psinger deleted the psi/simpo branch July 22, 2024 11:58
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.

[FEATURE] Implement SimPO
2 participants