Remove AugPipe

[ashnair1]

The next version of kornia (0.7.3) will contain 2 fixes (kornia/kornia#2846, kornia/kornia#2856) that will allow us to drop AugPipe and have detection only depend on kornia's AugmentationSequential.

[adamjstewart]

How close are we to also removing our custom AugmentationSequential wrapper?

[ashnair1]

How close are we to also removing our custom AugmentationSequential wrapper?

Pretty close actually. If kornia==0.7.3 is released soon, we should be able to fully remove it by next release.

[robmarkcole]

Noting https://github.com/kornia/kornia/releases/tag/v0.7.3

[adamjstewart]

@ashnair1 is this waiting on kornia 0.7.4 for something?

[ashnair1]

Yeah #2147 needs to be merged before this and that needs kornia 0.7.4.

[robmarkcole]

Note: 0.7.4 will be released by the end of September

[calebrob6]

This is good to go forward now

[adamjstewart]

Drop in coverage is due to some lines in our AugmentationSequential wrapper that are no longer hit. Can we just delete this wrapper now, or are we using it somewhere else? Ideally we would add some simple tests and deprecate it so as not to confuse too many people in the next release.

[ashnair1]

Drop in coverage is due to some lines in our AugmentationSequential wrapper that are no longer hit. Can we just delete this wrapper now, or are we using it somewhere else? Ideally we would add some simple tests and deprecate it so as not to confuse too many people in the next release.

No, the AugmentationSequential wrapper can be removed in this PR. I just needed to update tests to see if they were working as expected.

Have removed _ExtractPatches since kornia's ExtractTensorPatches works just as well.

[adamjstewart]

Alternatively, convert our AugmentationSequential to an alias of Kornia's so you don't need to add any tests. Or I can do that in my PR, but my PR can't be merged without this PR.

[ashnair1]

Making our AugmentationSequential an alias for kornia's won't work as _ExtractPatches won't work with it. It would have to be removed. Refer to #1978 (comment).

[adamjstewart]

Ah right. So we have to figure out how to fix or replace _ExtractPatches before either of our PRs can be merged 😢

[adamjstewart]

_ExtractPatches doesn't work correctly with kornia's AugmentationSequential.

Can you clarify this? I can hack this to work. Would like to push forward on this PR, either by adding more tests or fixing compatibility.

[ashnair1]

_ExtractPatches doesn't work correctly with kornia's AugmentationSequential.

Can you clarify this? I can hack this to work. Would like to push forward on this PR, either by adding more tests or fixing compatibility.

Turns out the test was incorrect. Our AugmentationSequential was changing the input batch itself and that's what we were testing. This PR should be good to go now. Though there does seem to be an issue with keepdim not being acknowledged correctly.

[adamjstewart]

Drop in coverage is for our AugmentationSequential. In torchgeo/transforms/transforms.py, can you replace our AugmentationSequential with:

from kornia.augmentation import AugmentationSequential

# Only include import redirects
__all__ = ('AugmentationSequential',)

I'll figure out the deprecation stuff in another PR.

[adamjstewart]

Well this is funny. The documentation tests are failing because it's trying to document Kornia's AugmentationSequential but we're missing an image file. In docs/api/transforms.rst, let's use:

.. automodule:: torchgeo.transforms
   :exclude-members: AugmentationSequential

[adamjstewart]

Thanks again for all your work on wrangling the sample dicts to have a consistent format! I'll start work on deprecation and type hints.

Tests have been updated