SSIM Modifications - Documentation Transparency and Return Consistency

[svarqq]

Description

Currently there is implicit internal reflection padding applied to images in computation of the full SSIM matrix, which is ignored via cropping in the computation of the returned mean. This PR addresses these issues by adding transparency about the padding in the docstring, and ensuring the mean returned is that of the full SSIM image.

[svarqq]

The change has expectedly broken a few tests. I'm adamant that the returned mean should be that of the returned SSIM image; with that in mind, there are a few ways to modify the implementation for cohesion with existing tests:

• Retain the current modification while updating all tests with argument full=True and post-call cropping on S for computing the mean expected by the test
• Make the function apply no padding, giving clients the option to manually pad before calling the function. This would require updating all tests that make use of the SSIM image, i.e. have argument full=True, but all tests making use only of the returned mean would pass
• Add a parameter to explicitly set padding; arguments could simply be the padding mode to be passed to the filter, with the default of no padding. Only tests making use of the SSIM image would have to be updated.

I think the third option is the best, as it allows manual prepadding in addition to the convenience of internal padding through an argument should a client want the SSIM image to be the same shape as the input images. By default the mean would retain the same value as prior.

Let me know what you think is best.

[stefanv]

I think the last option is the only one we can implement, because the others do not preserve backward compatibility. Out of curiosity, I looked on GH, and there are quite a few usages of this in the wild. I suspect that most uses of SSIM that request the image want to see where the differences are most prominent, but I haven't checked.

[svarqq]

Got it, I implemented the third with the default of no padding.

Note that backwards compatibility is broken for any current uses that rely on the SSIM image or gradient array being the same size as the inputs by default. Reflection padding could be set as the default to address that, but then the mean would include edge effects from padding which wasn't the default of the prior implementation which cropped them out. Sadly making the returned mean and SSIM image compatible changes the default return behaviour of prior.

Let me know what you think is best to handle this.

[stefanv]

The underlying principle is: code that ran before should continue running with the same output, or an error should be thrown.

I would suggest not throwing an error, just returning what was returned before.

You can add new padding modes and suggest best practices in the docstring, no problem.

[svarqq]

Okiedokes, I've patched it up so we retain the original default behaviour and faithfully honor the backwards compat contract with clients. Assuming all the CI checks pass without issue, take a look and let me know if it's all good now.

[svarqq]

Hey, just saw this. I've been super busy recently and don't see that letting up anytime soon, but will get around to making suggested changes in the new year. I also want to make sure I handled constant padding correctly.

I really appreciate the thoughtful review :).

[mkcor]

Hi @svarqq,

Are you available to wrap this up? Please let us know if you would like us to take over.

While working on #7813, I noticed it would be good to get this PR merged! Thank you.