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.

Sign up for GitHub

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

Jump to bottom

Remove AugPipe #1978

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Remove AugPipe #1978

wants to merge 5 commits into from

Conversation

ashnair1
Copy link
Collaborator

@ashnair1 ashnair1 commented Apr 3, 2024

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.

@github-actions github-actions bot added datasets Geospatial or benchmark datasets testing Continuous integration testing trainers PyTorch Lightning trainers datamodules PyTorch Lightning datamodules labels Apr 3, 2024
@adamjstewart
Copy link
Collaborator

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

@ashnair1
Copy link
Collaborator Author

ashnair1 commented Apr 3, 2024

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.

@adamjstewart adamjstewart mentioned this pull request Apr 5, 2024
Merged
@adamjstewart adamjstewart added this to the 0.6.0 milestone Apr 5, 2024
@ashnair1 ashnair1 mentioned this pull request Jun 9, 2024
Merged
@robmarkcole robmarkcole mentioned this pull request Jun 10, 2024
Open
@ashnair1 ashnair1 force-pushed the aug-remove branch 2 times, most recently from a018184 to 3cde2de Compare July 4, 2024 07:08
@robmarkcole
Copy link
Contributor

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

@adamjstewart
Copy link
Collaborator

@ashnair1 is this waiting on kornia 0.7.4 for something?

@ashnair1
Copy link
Collaborator Author

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

@adamjstewart adamjstewart modified the milestones: 0.6.0, 0.7.0 Aug 21, 2024
@robmarkcole
Copy link
Contributor

Note: 0.7.4 will be released by the end of September

@ashnair1 ashnair1 mentioned this pull request Nov 7, 2024
Draft
@ashnair1 ashnair1 force-pushed the aug-remove branch 3 times, most recently from 2d281ce to d60d6fb Compare November 9, 2024 19:09
@calebrob6
Copy link
Member

This is good to go forward now

@adamjstewart
Copy link
Collaborator

adamjstewart commented Dec 27, 2024 •
edited
Loading

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.

@github-actions github-actions bot added the transforms Data augmentation transforms label Dec 27, 2024
@ashnair1
Copy link
Collaborator Author

ashnair1 commented Dec 27, 2024 •
edited
Loading

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 ashnair1 changed the title Remove AugPipe Remove AugPipe and AugmentationSequential wrapper Dec 27, 2024
@ashnair1 ashnair1 marked this pull request as ready for review December 27, 2024 21:23
@isaaccorley
Copy link
Collaborator

We need to keep _ExtractPatches because kornia.contrib.ExtractTensorPatches doesn't inherit from the GeometricAugmentation so it won't apply to both image and masks when passing the batch dict.

isaaccorley
isaaccorley requested changes Dec 28, 2024
View reviewed changes
tests/transforms/test_transforms.py Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the transforms Data augmentation transforms label Dec 28, 2024
@ashnair1 ashnair1 changed the title Remove AugPipe and AugmentationSequential wrapper Remove AugPipe Dec 28, 2024
@ashnair1
Copy link
Collaborator Author

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

@isaaccorley
Copy link
Collaborator

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

@edgarriba
Copy link

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

@adamjstewart
Copy link
Collaborator

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
Copy link
Collaborator

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

@adamjstewart
Copy link
Collaborator

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

@ashnair1
Copy link
Collaborator Author

ashnair1 commented Dec 29, 2024 •
edited
Loading

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
adamjstewart reviewed Dec 30, 2024
View reviewed changes
torchgeo/trainers/detection.py
@@ -238,8 +238,12 @@ def training_step(
"""
x = batch['image']
batch_size = x.shape[0]
# Get bbox key as it can be one of {"bbox", "bbox_xyxy", "bbox_xywh"}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are our models compatible with all 3, or do we need to assert that a specific format is used?

tests/datasets/test_vhr10.py
assert isinstance(x['boxes'], torch.Tensor)
if 'masks' in x:
assert isinstance(x['masks'], torch.Tensor)
assert isinstance(x['class'], torch.Tensor)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the switch from labels to class? I don't think either are standard in Kornia

tests/datasets/test_vhr10.py
x['prediction_labels'] = x['labels']
x['prediction_boxes'] = x['boxes']
x['prediction_labels'] = x['class']
x['prediction_boxes'] = x['bbox_xyxy']
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also change the names of the prediction_* keys to match the new input keys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamjstewart adamjstewart adamjstewart left review comments

@isaaccorley isaaccorley isaaccorley requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
datamodules PyTorch Lightning datamodules datasets Geospatial or benchmark datasets testing Continuous integration testing trainers PyTorch Lightning trainers
Projects
None yet
Milestone
0.7.0
Development

Successfully merging this pull request may close these issues.
6 participants
@ashnair1 @adamjstewart @robmarkcole @calebrob6 @isaaccorley @edgarriba