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

re-implement the signal_distortion_ratio metric #964

Merged
merged 40 commits into from
Apr 21, 2022
Merged

re-implement the signal_distortion_ratio metric #964

merged 40 commits into from
Apr 21, 2022

Conversation

quancs
Copy link
Member

@quancs quancs commented Apr 17, 2022

What does this PR do?

Fixes #803

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
Copy link

codecov bot commented Apr 17, 2022

Codecov Report

Merging #964 (9343d5d) into master (b83edf0) will decrease coverage by 0%.
The diff coverage is 90%.

@@          Coverage Diff          @@
##           master   #964   +/-   ##
=====================================
- Coverage      95%    95%   -0%     
=====================================
  Files         177    177           
  Lines        7509   7513    +4     
=====================================
- Hits         7135   7126    -9     
- Misses        374    387   +13     

@quancs quancs changed the title reimplement the signal_distortion_ratio metric re-implement the signal_distortion_ratio metric Apr 17, 2022
@Borda Borda added the enhancement New feature or request label Apr 19, 2022
@Borda Borda modified the milestones: v0.9, v0.8 Apr 19, 2022
@Borda
Copy link
Member

Borda commented Apr 20, 2022

@quancs this is great! 💜

@quancs quancs marked this pull request as ready for review April 20, 2022 13:22
@quancs
Copy link
Member Author

quancs commented Apr 20, 2022

I assume we can safely remove this check right?

https://github.com/PyTorchLightning/metrics/blob/10b3a8529311579b3db1330842f730a643533065/torchmetrics/audio/sdr.py#L109-L113

else LGTM :]

Removed

@Borda Borda enabled auto-merge (squash) April 20, 2022 14:52
@Borda
Copy link
Member

Borda commented Apr 20, 2022

maybe we still need to patch some PT versions:

>       t_fft = torch.fft.rfft(target, n=n_fft, dim=-1)
E       AttributeError: 'builtin_function_or_method' object has no attribute 'rfft'

@mergify mergify bot added the ready label Apr 21, 2022
@Borda Borda merged commit f8ef656 into Lightning-AI:master Apr 21, 2022
Borda pushed a commit that referenced this pull request Apr 26, 2022
* reimplement signal_distortion_ratio
* sdr is differentiable for all supported pytorch version now
* update & fix
* chlog
* Apply suggestions from code review

Co-authored-by: quancs <quancs@qq.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Nicki Skafte Detlefsen <skaftenicki@gmail.com>

(cherry picked from commit f8ef656)
@Borda Borda added this to the v0.8 milestone May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

re-implement SDR
3 participants