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

Make SSIM work with NaN tests #52

Merged
merged 6 commits into from
Sep 11, 2024
Merged

Make SSIM work with NaN tests #52

merged 6 commits into from
Sep 11, 2024

Conversation

dfulu
Copy link
Collaborator

@dfulu dfulu commented Sep 4, 2024

Note: edited since first posted

By changing some of the parameters used in the structural_similarity() function from skimage, we can get the SSIM calculation to work with the examples in our tests which contain NaNs in the target image.

In working on this, we discovered that the old configuration of structural_similarity() only fails if there is a NaN in the top left (i.e. at y[..., 0, 0]) but doesn't fail if there are NaNs elsewhere. So we had only caught an edge case with our tests.

However, I believe that this new configuration of structural_similarity() which passes with our current tests is still better because it is more robust and more referenceable. The new configuration of structural_similarity() matches that used in the original paper . We use the hint in the structural_similarity() docstring to match to the original paper.

This solves #11

@dfulu dfulu requested a review from phinate September 4, 2024 12:15
@dfulu dfulu marked this pull request as ready for review September 4, 2024 12:23
@phinate
Copy link
Collaborator

phinate commented Sep 6, 2024

Just going through this to make sure I understand it. I thought I'd look at the SSIM between a random image from the leading channel and a blurry version to just see what happens. Top is the normal output of SSIM, bottom cuts out the border.

Here, the effect of NaNs appearing doesn't materialize. Do I need to be looking at all channels to see this working?

image

@dfulu dfulu changed the title Make SSIM work with NaNs Make SSIM work with NaN tests Sep 10, 2024
@dfulu dfulu requested a review from IFenton September 11, 2024 10:02
@IFenton
Copy link
Collaborator

IFenton commented Sep 11, 2024

LGTM

@phinate
Copy link
Collaborator

phinate commented Sep 11, 2024

So for documentation purposes, we had a discussion on this. Our tests were failing because of the very particular choice of putting a NaN in the top-left corner of our image:

ds["data"].values[:, :, 0, 0] = np.nan

There is some interaction here between this and setting gaussian_weights = False. We avoid this here by consistently using gaussian_weights = True everywhere, and fix the hyperparameters to match those in the original structural similarity paper. We didn't track down a concrete explanation of the all-NaN behaviour, but are satisfied that we now have a numerically stable output in the case of NaNs in the input!

@phinate phinate merged commit cd29ad7 into main Sep 11, 2024
7 checks passed
@phinate phinate deleted the ssim_nan branch September 11, 2024 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants