[Fix] CombineTensorPatches

[ashnair1]

Fixes #1557

Type of change

• 🧪 Tests Cases
• 🐞 Bug fix (non-breaking change which fixes an issue)
• 📝 This change requires a documentation update

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• Did you update CHANGELOG in case of a major change?

[ashnair1]

@edgarriba @shijianjian Any thoughts on this PR?

[edgarriba]

LGTM. Just comment on better variable naming. @shijianjian review and merge

[edgarriba]

I would add the last argument to keep backward compatibility

[ashnair1]

Since the function wasn't working correctly before, is there a need to maintain backwards compatibility?

[ashnair1]

While on the topic, wouldn't it better to remove the default arguments for combine and extract? While using these a user should explicitly designate the window_size, stride and original_size (for combine) parameters so that they get a reasonable result. Otherwise we run the risk of them specifying one parameter and getting an unexpected result or an OOM since default stride is 1.

[ashnair1]

Just found that this doesn't fully fix the problem. With non-zero padding the error still appears. I've added a test to account for this but I'll need to look into this a bit more to fully solve the issue.

[ashnair1]

Ok, think I've got it now. With the new fix, patching and merging works as expected even for rectangular images. Only point to note is that the original image dimensions should be even for unpadding to work correctly. Additionally the padded image size must be divisible by the window size.

@edgarriba @shijianjian Ready for review

[ashnair1]

Just noticed that this fix could also be ported to PatchSequential's restore_from_patches. Or we could just use combine_tensor_patches instead.

Edit: PatchSequential does not need this since it does the padding (if specified) and uses the corrected window_size before the extraction method. Refer here

[ashnair1]

Pinging @edgarriba @shijianjian

[edgarriba]

LGTM. @shijianjian merge once we have green lights in the CI

[shijianjian]

missing stride

[ashnair1]

combine_tensor_patches currently only supports stride == window_size. Refer here.

[edgarriba]

Might be worth to add a note somewhere about this detail

[ashnair1]

We could add the stride param and forward it to combine_tensor_patches where a NotImplementedError will be thrown.

[shijianjian]

I am wondering what is the definition of window_size here? why it changes so dramatically?window_size==image_size?

[ashnair1]

The window_size that is passed to combine_tensor_patches is the window_size used for extract_tensor_patches.
window_size in combine_tensor_patches is not the same as image size.

Consider the following code snippet

import torch

from kornia.contrib import CombineTensorPatches, ExtractTensorPatches

h, w = 8, 8
win = 4
pad = 2

image = torch.randn(1, 1, h, w)
print(image.shape)
tiler = ExtractTensorPatches(window_size=win, stride=win, padding=pad)
merger = CombineTensorPatches(original_size=(h, w), window_size=win,  unpadding=pad)
image_tiles = tiler(image)
print(image_tiles.shape)
new_image = merger(image_tiles)
print(new_image.shape)
assert (image == new_image).all()

Here
original_image_size = (8,8)
window_size used for extraction = (4, 4)
padding = (2,2,2,2)

Here window_size refers to size of the tiles we want. In this example, the extractor produces a tensor of shape (1, 9, 1, 4, 4). 9 tiles of size (4,4).

In combine_tensor_patches we compute window_size to take unpadding into account, so

combine_window_size = (
            (original_size[0] + (unpadding[0] + unpadding[1])) // window_size[0],
            (original_size[1] + (unpadding[2] + unpadding[3])) // window_size[1],
        )

which comes out to be (3, 3)

[edgarriba]

would nice to have in a posterior PR this snippet as an example with visualisation

[ashnair1]

Sure. I could do that in a follow up PR. Where do you recommend I add it?

[edgarriba]

Great, https://github.com/kornia/tutorials/tree/master/source