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

Added handling of case K1 = 0 and/or K2 = 0 in metrics.structural_similarity #5530

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Ouss98
Copy link

@Ouss98 Ouss98 commented Aug 23, 2021

The cases K1 = 0 and/or K2 = 0 gave NaNs in S if D had zeros.
Fixed by introducing appropriate factors S1 and S2, which
we set to 1 when zeros appear in the denominator.
This fixes the case when gradient is False.

Description

Checklist

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.

The cases K1 = 0 and/or K2 = 0 gave NaNs in S if D had zeros.
Fixed by introducing appropriate factors S1 and S2, which
we set to 1 when zeros appear in the denominator.
This fixes the case when gradient is False.
@pep8speaks
Copy link

pep8speaks commented Aug 23, 2021

Hello @Ouss98! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 230:1: W293 blank line contains whitespace
Line 233:1: W293 blank line contains whitespace
Line 237:1: W293 blank line contains whitespace
Line 239:1: W293 blank line contains whitespace

Comment last updated at 2021-08-30 09:52:26 UTC

@Ouss98
Copy link
Author

Ouss98 commented Sep 2, 2021

Please, let me know if there is anything I need to change or explain.

@rfezzani
Copy link
Member

rfezzani commented Sep 2, 2021

Sorry @Ouss98 for my late answer, and thank you for your contribution.
I am wondering if it is not simpler to forbid the cases K1=0 and K2=0: I see in the reference paper [1] that K1 and K2 are just some regularization parameters made to avoid 0 division!!

@rfezzani rfezzani changed the title Added handling of case K1 = 0 and/or K2 = 0 Added handling of case K1 = 0 and/or K2 = 0 in metrics.structural_similarity Sep 2, 2021
@grlee77
Copy link
Contributor

grlee77 commented Sep 2, 2021

I am wondering if it is not simpler to forbid the cases K1=0 and K2=0

I was thinking the same thing. We already have a check that they are not negative, so it seems simple to just extend this to forbid zero values and add a test case for that.

@grlee77 grlee77 added type: bug 🩹 type: Bug fix Fixes unexpected or incorrect behavior and removed type: bug labels Sep 2, 2021
@Ouss98
Copy link
Author

Ouss98 commented Sep 3, 2021

Thank you for your replies.
Sure, forbidding the case K1 or K2 = 0 can be a solution. However, in the reference paper C1 and C2 (thus K1 and K2) are only introduced to avoid instability of the algorithm.
In the method which I propose, the instability issue is avoided without taking into account arbitrary parameters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🩹 type: Bug fix Fixes unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants