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.

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

Refactor _RandomNCrop to RandomCrop + ExtractPatches #2682

Merged
merged 5 commits into from
Mar 31, 2025

Conversation

isaaccorley
Copy link
Collaborator

@isaaccorley isaaccorley commented Mar 27, 2025

This PR fixes #2667 and refactors _RandomNCrop out of segmentation and change detection datamodules in favor of K.RandomCrop during training and ExtractPatches for val/test/predicting.

@isaaccorley isaaccorley self-assigned this Mar 27, 2025
@github-actions github-actions bot added testing Continuous integration testing datamodules PyTorch Lightning datamodules labels Mar 27, 2025
@isaaccorley isaaccorley force-pushed the refactor/nrandomcrop branch from 0dc0f3f to 1a025c4 Compare March 27, 2025 01:50
@isaaccorley isaaccorley added this to the 0.7.0 milestone Mar 27, 2025
ashnair1
ashnair1 previously approved these changes Mar 27, 2025
@adamjstewart
Copy link
Collaborator

I would love to get rid of _ExtractPatches too, but I'm guessing kornia/kornia#1300 prevents us from doing that? It may be time to work more closely with the Kornia developers to upstream and replace our broken custom transforms.

@github-actions github-actions bot added the transforms Data augmentation transforms label Mar 27, 2025
@isaaccorley isaaccorley force-pushed the refactor/nrandomcrop branch from 2bd3a50 to b05719f Compare March 27, 2025 16:55
@isaaccorley
Copy link
Collaborator Author

I would love to get rid of _ExtractPatches too, but I'm guessing kornia/kornia#1300 prevents us from doing that? It may be time to work more closely with the Kornia developers to upstream and replace our broken custom transforms.

I'll bring that back from the grave after this gets merged. I want to see if they can help me with extracting patches but also being able to extract bboxes since this is important for object detection datasets that have large imagery. We don't want to lose users to SAHI.

Copy link
Collaborator

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

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

Due to the bugs for batch_size=1 and lack of support for bboxes, I'm strongly considering using RandomCrop for train and CenterCrop for val/test (with large patch size and small batch size) instead of replacing our hacky _RandomNCrop with another hacky _ExtractPatches. Then we can find time to contribute something better upstream to Kornia. How would you feel about that?

@isaaccorley
Copy link
Collaborator Author

isaaccorley commented Mar 28, 2025

I think _ExtractPatches is fine and likely more correct, particularly for datasets like LEVIR-CD where patch size is typically 256x256 and the image size is 1024x1024 so your validation and test metrics will actually encompass the entire test set images.

Using CenterCrop this would not be the case and not be able to provide an apples to apples comparison with other baselines.

@isaaccorley isaaccorley force-pushed the refactor/nrandomcrop branch from 91046e4 to 7d8a1a2 Compare March 29, 2025 18:32
@adamjstewart adamjstewart merged commit 83491ac into microsoft:main Mar 31, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datamodules PyTorch Lightning datamodules testing Continuous integration testing transforms Data augmentation transforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

_RandomNCrop does not work for batch_size > 1
3 participants