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.

Sign up for GitHub

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

Jump to bottom

Refactor: SNR & SI_SNR #712

Merged
merged 18 commits into from
Jan 8, 2022
Merged

Refactor: SNR & SI_SNR #712

merged 18 commits into from
Jan 8, 2022

Conversation

Borda
Copy link
Member

@Borda Borda commented Jan 5, 2022 •
edited
Loading

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 🙃

@Borda Borda added refactoring refactoring and code health API / design labels Jan 5, 2022
@Borda Borda self-assigned this Jan 5, 2022
@Borda Borda added this to the v0.7 milestone Jan 5, 2022
@codecov
Copy link

codecov bot commented Jan 5, 2022 •
edited
Loading

Codecov Report

Merging #712 (1fbff12) into master (fdf5b3f) will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #712   +/-   ##
=====================================
  Coverage      73%    73%           
=====================================
  Files         166    166           
  Lines        6465   6483   +18     
=====================================
+ Hits         4730   4748   +18     
  Misses       1735   1735

@justusschock justusschock mentioned this pull request Jan 5, 2022
Merged
4 tasks
@quancs
Copy link
Member

quancs commented Jan 5, 2022 •
edited
Loading

The signal noise ratio and signal distortion ratio are a little bit weird.

SNR is often called signal to noise ratio. And SDR is offen called signal to distortion ratio

@Borda Borda force-pushed the refactor/snr branch 2 times, most recently from 367fd59 to a2941cc Compare January 6, 2022 11:47
@Borda
Copy link
Member Author

Borda commented Jan 6, 2022

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

@Borda Borda requested a review from stancld January 6, 2022 14:25
@Borda
Copy link
Member Author

Borda commented Jan 6, 2022 •
edited
Loading

@Borda Maybe we can consider to put sisdr and sisnr in one file, because they have very limited difference (or to say sisnr is a special alias for sisdr)

I put sisnr in snr as it seems to be related, right?

Yes, sisnr and snr sounds more related. I'm wondering which is better:
1 put sisnr and snr together;
2 put sisnr and sisdr together;
3 put sisnr, sisdr, snr separately as before

I would go with 1

@Borda Borda marked this pull request as ready for review January 6, 2022 23:39
@Borda Borda requested a review from quancs January 7, 2022 09:10
@Borda Borda mentioned this pull request Jan 7, 2022
Closed
12 tasks
@Borda Borda marked this pull request as draft January 7, 2022 09:55
@Borda Borda marked this pull request as ready for review January 8, 2022 10:49
@Borda Borda enabled auto-merge (squash) January 8, 2022 10:57
@mergify mergify bot added the ready label Jan 8, 2022
Borda
Borda commented Jan 8, 2022
View reviewed changes
torchmetrics/audio/si_snr.py Outdated Show resolved Hide resolved
Borda
Borda commented Jan 8, 2022
View reviewed changes
torchmetrics/audio/snr.py Outdated Show resolved Hide resolved
@mergify mergify bot added the has conflicts label Jan 8, 2022
@mergify mergify bot removed the has conflicts label Jan 8, 2022
@Borda Borda disabled auto-merge January 8, 2022 20:25
@Borda Borda merged commit b7518dd into master Jan 8, 2022
@Borda Borda deleted the refactor/snr branch January 8, 2022 20:25
Borda added a commit that referenced this pull request Jan 8, 2022
@Borda @pre-commit-ci
Refactor: SNR & SI_SNR (#712)
d11641c 
* signal_noise_ratio
* scale_invariant_signal_noise_ratio
* SignalNoiseRatio
* ScaleInvariantSignalNoiseRatio

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Borda added a commit to AIMistakes/metrics that referenced this pull request Jan 10, 2022
@Borda @pre-commit-ci
Refactor: SNR & SI_SNR (Lightning-AI#712)
48d51c2 
* signal_noise_ratio
* scale_invariant_signal_noise_ratio
* SignalNoiseRatio
* ScaleInvariantSignalNoiseRatio

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quancs quancs quancs approved these changes

@SkafteNicki SkafteNicki SkafteNicki approved these changes

@ananyahjha93 ananyahjha93 Awaiting requested review from ananyahjha93

@edenlightning edenlightning Awaiting requested review from edenlightning

@ethanwharris ethanwharris Awaiting requested review from ethanwharris

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@tchaton tchaton Awaiting requested review from tchaton

@stancld stancld Awaiting requested review from stancld stancld is a code owner

@lucadiliello lucadiliello Awaiting requested review from lucadiliello

@justusschock justusschock Awaiting requested review from justusschock justusschock is a code owner

Assignees

@Borda Borda

Labels
API / design ready refactoring refactoring and code health
Projects
None yet
Milestone
v0.7
Development

Successfully merging this pull request may close these issues.
6 participants
@Borda @quancs @justusschock @lucadiliello @SkafteNicki @stancld