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.

[ashnair1]

Scoping this back to just removing AugPipe. AugmentationSequential can't be removed yet as _ExtractPatches doesn't work correctly with kornia's AugmentationSequential.

[isaaccorley]

@adamjstewart Do we want to remove _ExtractPatches and I can make a PR in kornia for an ExtractTensorPatches augmentation? Pinging @edgarriba for awareness.

[edgarriba]

@isaaccorley sounds good to me. Feel free to join our discourse to chat about it: https://discord.gg/Vf3QgRq2 @johnnv1

[adamjstewart]

I would like to remove everything in torchgeo/transforms/transforms.py sooner rather than later. So yes, anything anyone is willing to upstream I would be happy to help review. In particular, I'm also interested in upstreaming RandomNCrop. We noticed a bug in it when using data with a time dimension so we just need to fix that.

[isaaccorley]

Then yes sorry @ashnair1, let's remove AugmentationSequential and _ExtractPatches

[adamjstewart]

Or deprecate. It may take a while for upstream to make a new release.

[ashnair1]

Or deprecate. It may take a while for upstream to make a new release.

I think it might be better to keep the current scope of the PR as is i.e. removal of AugPipe. Deprecation can be done in #2396 right?

[adamjstewart]

One minor undo and then I think this is ready to merge!

[adamjstewart]

Drop in coverage is for AugmentationSequential: https://app.codecov.io/gh/microsoft/torchgeo/pull/1978/blob/torchgeo/transforms/transforms.py

Can you add a couple tests?

[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 😢