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

update SISNR doc to make it clear we display minus the SISNR #268

Merged
merged 3 commits into from
Sep 7, 2023

Conversation

adefossez
Copy link
Contributor

we report minus the SISNR so that it can also be used as a training loss, but that can be confusing !

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 7, 2023
Copy link
Contributor

@JadeCopet JadeCopet left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -41,6 +41,9 @@ class SISNR(nn.Module):

Input should be [B, C, T], output is scalar.

..Warning:: We report here minus SISNR, so negative values are better. This is so that
this can be naturally used as a loss!

Copy link
Contributor

Choose a reason for hiding this comment

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

@adefossez i kindly request to update the following warning messages, these updates are based on my understanding and aim to avoid confusion when reading the documentation. i apologize for any inconvenience caused by my lack of understanding. thank you for considering my request.

eg: ..Warning:: The reported values presented here represent the negation of SISNR. Consequently, negative values indicate better results. This format is intentionally designed to facilitate the convenient usage of SISNR as a loss function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i have extended a bit the warnings

@@ -16,6 +16,8 @@ We provide an implementation of the Scale-Invariant Signal-to-Noise Ratio in PyT
No specific requirement is needed for this metric. Please activate the metric at the
evaluation stage with the appropriate flag:

**Warning:** As we also allow SISNR to be used as a training loss, we actually report minus the SISNR, e.g. negative values are best.

Copy link
Contributor

Choose a reason for hiding this comment

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

continued:
Warning Please be aware that in the case of using SISNR as a training loss, the reported values are represented as negative. This implies that the best results are indicated by lower (negative) values.

assuming they do not change the original intention or message.

@adefossez adefossez merged commit 80def9c into main Sep 7, 2023
3 checks passed
Dinglet pushed a commit to Dinglet/audiocraft that referenced this pull request Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants