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

Add NISQA metric #2792

Merged
merged 27 commits into from
Oct 30, 2024
Merged

Add NISQA metric #2792

merged 27 commits into from
Oct 30, 2024

Conversation

philgzl
Copy link
Contributor

@philgzl philgzl commented Oct 21, 2024

What does this PR do?

This PR adds a new metric NISQA as suggested in #2464

Before submitting
  • Was this discussed/agreed 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 🙃


📚 Documentation preview 📚: https://torchmetrics--2792.org.readthedocs.build/en/2792/

@github-actions github-actions bot added documentation Improvements or additions to documentation topic: Audio labels Oct 21, 2024
Copy link

codecov bot commented Oct 21, 2024

Codecov Report

Attention: Patch coverage is 91.93548% with 20 lines in your changes missing coverage. Please review.

Project coverage is 35%. Comparing base (429556b) to head (2b34960).
Report is 1 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (429556b) and HEAD (2b34960). Click for more details.

HEAD has 4 uploads less than BASE
Flag BASE (429556b) HEAD (2b34960)
gpu 2 0
unittest 2 0
Additional details and impacted files
@@           Coverage Diff            @@
##           master   #2792     +/-   ##
========================================
- Coverage      69%     35%    -33%     
========================================
  Files         340     328     -12     
  Lines       18448   18525     +77     
========================================
- Hits        12679    6539   -6140     
- Misses       5769   11986   +6217     

@SkafteNicki SkafteNicki self-assigned this Oct 22, 2024
@SkafteNicki SkafteNicki added this to the v1.6.0 milestone Oct 22, 2024
Copy link
Member

@SkafteNicki SkafteNicki left a comment

Choose a reason for hiding this comment

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

Hi @philgzl, thanks for this contribution to TM.
I am overall very happy with the PR already, it is nearly complete. If I could ask you to add some doc strings (just one liners is fine) to the different functions/methods in src/torchmetrics/functional/audio/nisqa.py that would be great for later maintaining the code.

src/torchmetrics/audio/nisqa.py Show resolved Hide resolved
src/torchmetrics/functional/audio/nisqa.py Show resolved Hide resolved
Comment on lines +130 to +133
# main NISQA model definition
# ported from https://github.com/gabrielmittag/NISQA
# Copyright (c) 2021 Gabriel Mittag, Quality and Usability Lab
# MIT License
Copy link
Member

Choose a reason for hiding this comment

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

@Borda we had copyright issue before, anything we need to do here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The model weights are also under CC BY-NC-SA license

Copy link
Member

@Borda Borda Oct 22, 2024

Choose a reason for hiding this comment

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

we will need to get the authors to allow distribute it with an Apache licence
od can we import as an optional dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Original code is not a pip installable package so can't really import as an optional dependency.

DNSMOS is similar: MIT license code and CC BY license weights. How was this managed? Or was this overlooked?

tests/unittests/audio/test_nisqa.py Outdated Show resolved Hide resolved
@mergify mergify bot added ready and removed has conflicts labels Oct 22, 2024
Borda
Borda previously requested changes Oct 22, 2024
Comment on lines +130 to +133
# main NISQA model definition
# ported from https://github.com/gabrielmittag/NISQA
# Copyright (c) 2021 Gabriel Mittag, Quality and Usability Lab
# MIT License
Copy link
Member

@Borda Borda Oct 22, 2024

Choose a reason for hiding this comment

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

we will need to get the authors to allow distribute it with an Apache licence
od can we import as an optional dependency?

@mergify mergify bot added has conflicts and removed ready labels Oct 22, 2024
@mergify mergify bot removed the has conflicts label Oct 29, 2024
@Borda
Copy link
Member

Borda commented Oct 29, 2024

hote this is correct:

If a package with an Apache License includes code that is under an MIT License, the combined project needs to respect both licenses. Here are the key implications:

  • Compatibility: The Apache License 2.0 and MIT License are compatible, meaning code licensed under MIT can legally be included in a project under the Apache License.

  • License Notices: Since the MIT License requires preservation of its copyright and permission notice, these must be retained in the source files or documentation. In addition, the Apache License also requires its own licensing notice and attribution, so both notices should be clearly included.

  • Patent Grant: The Apache License includes a patent grant, while the MIT License does not. If you're using MIT-licensed code in a project that you distribute under the Apache License, the Apache License's patent grant will only cover the parts of the code that are explicitly Apache-licensed, not the portions under MIT. However, this doesn’t prevent you from distributing the MIT code within the Apache-licensed package.

  • Redistribution Terms: The resulting combined work can still be redistributed under the terms of the Apache License, as long as both licenses are respected in the documentation. Since both licenses are permissive, users are generally free to use, modify, and distribute the combined package, but the Apache License will apply to the package as a whole.

In practice, the combination works smoothly, and users of the package can use it as long as they comply with both license terms—chiefly by keeping both license texts intact and visible

@Borda Borda self-requested a review October 29, 2024 18:15
@mergify mergify bot added the ready label Oct 29, 2024
@SkafteNicki
Copy link
Member

@Borda added full copyright at the top of the affected file, so PR should be good to go now

@Borda Borda merged commit 76c502b into Lightning-AI:master Oct 30, 2024
55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation ready topic: Audio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants