Refactor: SDR & SI_SDR #711

Borda · 2022-01-05T00:14:38Z

What does this PR do?

Unify the notation and keep more descriptive functional names
part of #729

Before submitting

Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
Did you read the contributor guideline, Pull Request section?
Did you make sure to update the docs?
Did you write any new necessary tests?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

codecov · 2022-01-05T00:18:20Z

Codecov Report

Merging #711 (fb39eb0) into master (2f32c2d) will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #711   +/-   ##
=====================================
  Coverage      95%    95%           
=====================================
  Files         166    166           
  Lines        6448   6465   +17     
=====================================
+ Hits         6137   6154   +17     
  Misses        311    311

CHANGELOG.md

quancs · 2022-01-05T11:05:41Z

@Borda why we need to rename SDR & SISDR? SDR and SISDR are both commonly used name in audio field. It's short and easy to type

Borda · 2022-01-05T11:08:19Z

SDR and SISDR are both commonly used name in audio field. It's short and easy to type

that is the point that they are common in audio, but then we import them in all metrics levels... 🐰
in fact, I am fine with SDR, but the SI is a bit confusing
my main concern is having _ in the class name is not pythonic and inconsistent with other metrics

quancs · 2022-01-05T12:15:37Z

that is the point that they are common in audio, but then we import them in all metrics levels... 🐰 in fact, I am fine with SDR, but the SI is a bit confusing my main concern is having _ in the class name is not pythonic and inconsistent with other metrics

Importing sdr and si_sdr in all metrics levels only make troubles when some other metrics have the same short name with them. After googling SDR, I can only find Special Drawing Rights (SDRs) , software defined Radio (SDR), Sales Development Representatives (SDRs) in the first three pages. If you think it's a confusing name, can you give some examples?

Below is the original definition of SI-SDR in paper.

The - cannot be used, thus _ is used instead. I know it's not pythonic, but add a separator is the tradition in audio field. I can list many and many papers to demonstrate that.

Borda · 2022-01-05T14:14:16Z

@quancs first of all, it is not meant as any form of offense to audio metrics or lowering your contribution! I just try to find the best way for TorchMterics as a coherent package and have a consistent name convention...

as we have also MAPE, which is also most likely only one meaning, but we call it Mean absolute percentage error

quancs · 2022-01-05T15:37:03Z

@Borda I was a little bit hurry before. ^^
Removing the _ is OK for me. But I really don't like the idea to expand the names of audio metrics. The short names are unique, clear and concise in audio field.

for more information, see https://pre-commit.ci

Borda · 2022-01-06T12:00:23Z

@PyTorchLightning/core-metrics updated and keeping abbreviations, pls check it now 🐰

Borda · 2022-01-07T09:12:26Z

@quancs is there a reason why you are moving it back to separate files?
I thought that we (I suggested and you did not dispute #712 (comment)) agreed to have it in the same file

quancs · 2022-01-07T17:38:56Z

@quancs is there a reason why you are moving it back to separate files? I thought that we (I suggested and you did not dispute #712 (comment)) agreed to have it in the same file

I remembered that you said you would go with 2 ( 2 put sisnr and sisdr together;), so ...

quancs · 2022-01-07T17:40:42Z

@Borda I thought that's our agreement ... So, you changed your mind? ^^

quancs · 2022-01-07T17:43:33Z

I'm OK with both choices. You can choose the anyone you like.

quancs · 2022-01-07T17:48:54Z

@Borda Another decision we need to make is about the full name of SDR and SNR. signal_to_noise_ratio or signal_noise_ratio? signal_to_distortion_ratio or signal_distortion_ratio? What do you think

Borda · 2022-01-07T17:51:22Z

@Borda Another decision we need to make is about the full name of SDR and SNR. signal_to_noise_ratio or signal_noise_ratio? signal_to_distortion_ratio or signal_distortion_ratio? What do you think

I would follow sklearn and skimage and drite it without _to_

I remembered that you said you would go with 2 ( 2 put sisnr and sisdr together;), so ...

I made typo, then fixed it, but in all conversations, I was saying to put sdr and si-sdr together as it rather a variant of sdr the si

I'm OK with both choices. You can choose the anyone you like.

lets stick with the resolution on metrics slack

quancs · 2022-01-07T17:54:20Z

@Borda Another decision we need to make is about the full name of SDR and SNR. signal_to_noise_ratio or signal_noise_ratio? signal_to_distortion_ratio or signal_distortion_ratio? What do you think

I would follow sklearn and skimage and drite it without _to_

I remembered that you said you would go with 2 ( 2 put sisnr and sisdr together;), so ...

I made typo, then fixed it, but in all conversations, I was saying to put sdr and si-sdr together as it rather a variant of sdr the si

I'm OK with both choices. You can choose the anyone you like.

lets stick with the resolution on metrics slack

OK. Agree.

torchmetrics/functional/audio/si_sdr.py

for more information, see https://pre-commit.ci

* signal_distortion_ratio * scale_invariant_signal_distortion_ratio * SignalDistortionRatio * ScaleInvariantSignalDistortionRatio Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Borda added refactoring refactoring and code health API / design labels Jan 5, 2022

Borda requested review from ananyahjha93, edenlightning, ethanwharris, justusschock, SeanNaren, SkafteNicki and tchaton as code owners January 5, 2022 00:14

Borda self-assigned this Jan 5, 2022

Borda added this to the v0.7 milestone Jan 5, 2022

justusschock reviewed Jan 5, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

mergify bot added has conflicts and removed has conflicts labels Jan 6, 2022

Borda and others added 8 commits January 6, 2022 12:34

signal_distortion_ratio

97f1250

cleaning

c472869

ScaleInvariantSDR

33c6771

[pre-commit.ci] auto fixes from pre-commit.com hooks

37d50d9

for more information, see https://pre-commit.ci

chlog

382213b

.

5ac8024

.

e20481e

..

060d116

Borda force-pushed the refactor/sdr branch 2 times, most recently from 29e71ff to 8285d95 Compare January 6, 2022 11:54

Borda requested a review from justusschock January 6, 2022 14:25

Borda mentioned this pull request Jan 6, 2022

Refactor: SNR & SI_SNR #712

Merged

4 tasks

Borda force-pushed the refactor/sdr branch from b7b8355 to 0033c3b Compare January 7, 2022 09:14

Borda mentioned this pull request Jan 7, 2022

Unify functional/class metric's names #729

Closed

12 tasks

Borda marked this pull request as draft January 7, 2022 09:55

Borda force-pushed the refactor/sdr branch from 0033c3b to 060d116 Compare January 7, 2022 13:25

quancs reviewed Jan 7, 2022

View reviewed changes

torchmetrics/functional/audio/si_sdr.py Show resolved Hide resolved

quancs approved these changes Jan 7, 2022

View reviewed changes

Borda and others added 3 commits January 8, 2022 11:34

SignalDistortionRatio

46cbafa

ScaleInvariantSignalDistortionRatio

df6f123

[pre-commit.ci] auto fixes from pre-commit.com hooks

7a7fd7b

for more information, see https://pre-commit.ci

Borda marked this pull request as ready for review January 8, 2022 10:49

Merge branch 'master' into refactor/sdr

fb39eb0

Borda enabled auto-merge (squash) January 8, 2022 10:56

mergify bot added the ready label Jan 8, 2022

SkafteNicki approved these changes Jan 8, 2022

View reviewed changes

Borda disabled auto-merge January 8, 2022 19:33

Borda merged commit fdf5b3f into master Jan 8, 2022

Borda deleted the refactor/sdr branch January 8, 2022 19:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: SDR & SI_SDR #711

Refactor: SDR & SI_SDR #711

Borda commented Jan 5, 2022 •

edited

Loading

codecov bot commented Jan 5, 2022 •

edited

Loading

quancs commented Jan 5, 2022 •

edited

Loading

Borda commented Jan 5, 2022 •

edited

Loading

quancs commented Jan 5, 2022 •

edited

Loading

Borda commented Jan 5, 2022 •

edited

Loading

quancs commented Jan 5, 2022

Borda commented Jan 6, 2022

Borda commented Jan 7, 2022 •

edited

Loading

quancs commented Jan 7, 2022

quancs commented Jan 7, 2022

quancs commented Jan 7, 2022

quancs commented Jan 7, 2022

Borda commented Jan 7, 2022

quancs commented Jan 7, 2022

Refactor: SDR & SI_SDR #711

Refactor: SDR & SI_SDR #711

Conversation

Borda commented Jan 5, 2022 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

codecov bot commented Jan 5, 2022 • edited Loading

Codecov Report

quancs commented Jan 5, 2022 • edited Loading

Borda commented Jan 5, 2022 • edited Loading

quancs commented Jan 5, 2022 • edited Loading

Borda commented Jan 5, 2022 • edited Loading

quancs commented Jan 5, 2022

Borda commented Jan 6, 2022

Borda commented Jan 7, 2022 • edited Loading

quancs commented Jan 7, 2022

quancs commented Jan 7, 2022

quancs commented Jan 7, 2022

quancs commented Jan 7, 2022

Borda commented Jan 7, 2022

quancs commented Jan 7, 2022

Borda commented Jan 5, 2022 •

edited

Loading

codecov bot commented Jan 5, 2022 •

edited

Loading

quancs commented Jan 5, 2022 •

edited

Loading

Borda commented Jan 5, 2022 •

edited

Loading

quancs commented Jan 5, 2022 •

edited

Loading

Borda commented Jan 5, 2022 •

edited

Loading

Borda commented Jan 7, 2022 •

edited

Loading