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 Visual Information Fidelity #1830

Merged
merged 36 commits into from
Jul 11, 2023
Merged

Add Visual Information Fidelity #1830

merged 36 commits into from
Jul 11, 2023

Conversation

bojobo
Copy link
Contributor

@bojobo bojobo commented Jun 7, 2023

What does this PR do?

Adds metric visual information fidelity (vif). See #799

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--1830.org.readthedocs.build/en/1830/

@SkafteNicki
Copy link
Member

@bojobo cool adding, feel free to reach out if you need help with anything :]

@bojobo
Copy link
Contributor Author

bojobo commented Jun 16, 2023

Will do that, thanks :)
I'll try to be done by next week. Is that early enough to get this metric into the next release? @SkafteNicki

@bojobo
Copy link
Contributor Author

bojobo commented Jun 17, 2023

@SkafteNicki

I'm sure that I'm on the right path, but I can't quite get the same results as the packages I'm using for reference.
My implementation is based on the implementation found in sewar as well as on the implementation found in piq. Furthermore, to make sure that the implementation is same as the paper, I've used the published resources (where this .zip contains the MatLab implementation). I've managed to create the same filter as sewer, but I can't get the same result.

Do you see any differences in the implementation? Or any fatal errors?

Also, I'm not quite sure how to implement the tests, since I've almost never done that (I know, I know, shame on me). I'm grateful for any form of help :)

@stancld stancld added this to the v1.1.0 milestone Jun 21, 2023
@SkafteNicki
Copy link
Member

SkafteNicki commented Jun 28, 2023

Hi @bojobo, sorry for not responding sooner.
I tried out the code in PR and cannot see a problem with the implementation. Here is the testing script I used:

import sewar
import torch
import numpy as np

preds = np.random.randint(0, 255, (1, 41, 41, 1), dtype=np.uint8)
target = np.random.randint(0, 255, (1, 41, 41, 1), dtype=np.uint8)

compare = sewar.full_ref.vifp(target.squeeze(), preds.squeeze())


from torchmetrics.functional.image.vif import visual_information_fidelity

tm_res = visual_information_fidelity(
    torch.tensor(preds).permute(0,3,1,2),
    torch.tensor(target).permute(0,3,1,2),
    data_range=255.0
).item()

print(compare)
print(tm_res)
print(abs(compare - tm_res))

I get a difference in result of 1e-7 to 1e-9. So the implementation seems to be working to me.

@bojobo
Copy link
Contributor Author

bojobo commented Jul 3, 2023

@SkafteNicki Thanks for the reply!
I have some follow-up questions.

  • I don't understand why mypy is failing. I understand the returned message, but if I'm correct, other metrics are computed similarly and don't fail.
  • ddp=True leads to failing tests. I don't know what I'm missing to make it work. I would appreciate any help.

@SkafteNicki
Copy link
Member

@SkafteNicki Thanks for the reply! I have some follow-up questions.

* I don't understand why `mypy` is failing. I understand the returned message, but if I'm correct, other metrics are computed similarly and don't fail.

* `ddp=True` leads to failing tests. I don't know what I'm missing to make it work. I would appreciate any help.

Hi @bojobo, I have fixed the typing issue. These two lines were missing:

vif_score: Tensor
total: Tensor

which informs mypy about the type of the metric states.

For the ddp, I have to look further into that.

@SkafteNicki SkafteNicki mentioned this pull request Jul 6, 2023
@SkafteNicki SkafteNicki marked this pull request as ready for review July 9, 2023 14:17
@codecov
Copy link

codecov bot commented Jul 10, 2023

Codecov Report

Merging #1830 (55fbb2c) into master (3fed40f) will decrease coverage by 42%.
The diff coverage is 95%.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1830     +/-   ##
========================================
- Coverage      85%     43%    -42%     
========================================
  Files         260     262      +2     
  Lines       14912   14998     +86     
========================================
- Hits        12669    6378   -6291     
- Misses       2243    8620   +6377     

@mergify mergify bot added the ready label Jul 10, 2023
@Borda Borda enabled auto-merge (squash) July 11, 2023 11:53
@Borda Borda merged commit 93ac13f into Lightning-AI:master Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants