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

Better loss system for broadcast_loss #126

Merged
merged 3 commits into from
Sep 19, 2024
Merged

Better loss system for broadcast_loss #126

merged 3 commits into from
Sep 19, 2024

Conversation

bdvllrs
Copy link
Collaborator

@bdvllrs bdvllrs commented Aug 13, 2024

if compute_loss returns None, it's skipped + Broadcast loss uses tr, dcy, cy and fused losses

@bdvllrs bdvllrs force-pushed the optional_domain_loss branch from 8d4184c to 15fae9b Compare September 19, 2024 07:59
@RolandBERTINJOHANNET
Copy link
Collaborator

In which situation would compute_loss return none ? Wouldn't we want an error in this case ?
Also I just want to check, the functions compute_[...]_loss are added in this PR so we can choose to have different computations based on the loss type ?

@bdvllrs
Copy link
Collaborator Author

bdvllrs commented Sep 19, 2024

In which situation would compute_loss return none ? Wouldn't we want an error in this case ?

The rationale is that a domain module can choose to never have one of the loss (e.g. to not participate to translations).
Choosing a loss coef to 0 is a user choice, setting one loss to None is a domain choice. It obviously has priority over a non 0 loss coef.

Also I just want to check, the functions compute_[...]_loss are added in this PR so we can choose to have different computations based on the loss type ?

Those were not added in this PR and have been part of shimmer for some times already, but it was not used by the broadcast loss. But basically yes, it's useful in case translation and demi cycles not use the same exact loss function.

Copy link
Collaborator

@RolandBERTINJOHANNET RolandBERTINJOHANNET left a comment

Choose a reason for hiding this comment

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

OK makes sense :)

@bdvllrs
Copy link
Collaborator Author

bdvllrs commented Sep 19, 2024

I added documented the change and explicited what I just wrote. I agree, it wasn't clear originally.

@bdvllrs bdvllrs merged commit 2c9b60d into main Sep 19, 2024
1 check passed
@bdvllrs bdvllrs deleted the optional_domain_loss branch September 19, 2024 09:27
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.

2 participants