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

Adding psnrb #1421

Merged
merged 50 commits into from Apr 17, 2023
Merged

Adding psnrb #1421

merged 50 commits into from Apr 17, 2023

Conversation

soma2000-lang
Copy link
Contributor

@soma2000-lang soma2000-lang commented Jan 1, 2023

What does this PR do?

Part of #799

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 🙃

@soma2000-lang
Copy link
Contributor Author

@Borda @SkafteNicki Any help regarding the failing tests would be really helpful

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.

Some comments, already looking good.
Please also add entry in changelog :)

docs/source/image/peak_signl_to_noise Outdated Show resolved Hide resolved
docs/source/links.rst Outdated Show resolved Hide resolved
src/torchmetrics/functional/image/psnrb.py Outdated Show resolved Hide resolved
src/torchmetrics/image/psnrb.py Outdated Show resolved Hide resolved
src/torchmetrics/functional/image/psnrb.py Outdated Show resolved Hide resolved
src/torchmetrics/functional/image/psnrb.py Outdated Show resolved Hide resolved
src/torchmetrics/functional/image/psnrb.py Outdated Show resolved Hide resolved
src/torchmetrics/functional/image/psnrb.py Outdated Show resolved Hide resolved
@soma2000-lang
Copy link
Contributor Author

image

@SkafteNicki I have almost made all the changes as you suggested ,but I require some help as to what might have caused this error while running pytest

@soma2000-lang
Copy link
Contributor Author

@Borda @SkafteNicki I cannot understand why its still giving the import error,even after the init file has been fixed.

@Borda
Copy link
Member

Borda commented Jan 6, 2023

@Borda @SkafteNicki I cannot understand why its still giving the import error,even after the init file has been fixed.

seems you used the previous PSNR but did not update the class name, also curious if we could rather reuse some functionality instead of writing all from the ground... 🐰

@mergify mergify bot removed the has conflicts label Apr 14, 2023
@codecov
Copy link

codecov bot commented Apr 14, 2023

Codecov Report

Merging #1421 (8181e61) into master (527afbe) will decrease coverage by 45%.
The diff coverage is 30%.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1421     +/-   ##
========================================
- Coverage      88%     42%    -45%     
========================================
  Files         238     240      +2     
  Lines       13360   13510    +150     
========================================
- Hits        11741    5738   -6003     
- Misses       1619    7772   +6153     

@Borda Borda requested a review from stancld April 15, 2023 19:23
@stancld stancld enabled auto-merge (squash) April 15, 2023 19:37
Image processing automation moved this from In progress to Reviewer approved Apr 15, 2023
@mergify mergify bot added the ready label Apr 16, 2023
@stancld stancld merged commit eeea871 into Lightning-AI:master Apr 17, 2023
62 checks passed
Image processing automation moved this from Reviewer approved to Done Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants