-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Min-SNR Gamma: correct the fix for SNR weighted loss in v-prediction … #5238
Min-SNR Gamma: correct the fix for SNR weighted loss in v-prediction … #5238
Conversation
…by adding 1 to SNR rather than the resulting loss weights
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for iterating!
@bghira Doesn't this change reintroduce the division by zero issue when using zero terminal SNR + epsilon prediction? As is, the weight will be |
in my opinion, you shouldn't be combining zero terminal SNR with epsilon anyway. i'm sure there's an additional fix that can be applied, but this trainer uses the simplest implementations of the logic from the research paper. using epsilon would be out of scope for that? @sayakpaul thoughts on that? |
Yeah I agree. |
huggingface#5238) Min-SNR Gamma: correct the fix for SNR weighted loss in v-prediction by adding 1 to SNR rather than the resulting loss weights Co-authored-by: bghira <[email protected]> Co-authored-by: Sayak Paul <[email protected]>
…by adding 1 to SNR rather than the resulting loss weights
What does this PR do?
Fixes issue as mentioned by @parlance-zz here.
Sorry for all the back and forth on this stuff, I did about 6000 steps of training with the previous fix and all was well. If this works better, awesome!