Skip to content

Fix #1274: Rename and fix PadIfNeeded to only pad (no crop)#1296

Closed
abhayrajjais01 wants to merge 5 commits intoweecology:mainfrom
abhayrajjais01:fix/issue-1272-padifneeded
Closed

Fix #1274: Rename and fix PadIfNeeded to only pad (no crop)#1296
abhayrajjais01 wants to merge 5 commits intoweecology:mainfrom
abhayrajjais01:fix/issue-1272-padifneeded

Conversation

@abhayrajjais01
Copy link
Copy Markdown
Contributor

@abhayrajjais01 abhayrajjais01 commented Feb 2, 2026

Summary

This PR fixes Issue #1274 where PadIfNeeded was incorrectly cropping images larger than the target size because it was aliased directly to Kornia.PadTo.

Changes

  • New PadIfNeeded Class: Implemented a custom PadIfNeeded augmentation (inheriting from GeometricAugmentationBase2D) that:
    • Checks if the input image dimensions are smaller than the target size.
    • Pads only if needed.
    • Returns the identity (unchanged image) if the image is already larger than the target size.
  • Added PadTo: Added a new key "PadTo" to _SUPPORTED_TRANSFORMS that maps to the original Kornia.PadTo behavior. This preserves backward compatibility for users who relied on the cropping behavior.
  • Tests: Added a new regression test test_pad_if_needed in tests/test_augmentations.py to verify:
    • Small images are padded correctly.
    • Large images are NOT cropped (the fix).
    • PadTo still works as expected (legacy behavior).

Verification

Ran tests via Docker:
docker compose run --rm deepforest pytest tests/test_augmentations.py
Result: All 28 tests passed.

I have taken help of ai to understand the codebase

- Distribute padding evenly on all sides (left/right, top/bottom)
- Keeps image centered to preserve spatial alignment
- Addresses feedback from @Prathamesh8989 in PR weecology#1289
@abhayrajjais01 abhayrajjais01 force-pushed the fix/issue-1272-padifneeded branch from 42ee699 to c0f0e17 Compare February 2, 2026 16:48
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 2, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.41%. Comparing base (ecf3a1c) to head (a97b9b7).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1296      +/-   ##
==========================================
+ Coverage   87.34%   87.41%   +0.07%     
==========================================
  Files          24       24              
  Lines        2978     2996      +18     
==========================================
+ Hits         2601     2619      +18     
  Misses        377      377              
Flag Coverage Δ
unittests 87.41% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Changed default p from 0.5 to 1.0 to ensure padding always applies
- Fixes CI test failures where augmentation was randomly skipping
- All 28 augmentation tests now pass consistently
@abhayrajjais01 abhayrajjais01 force-pushed the fix/issue-1272-padifneeded branch from ae5f444 to 7195348 Compare February 6, 2026 05:15
@abhayrajjais01
Copy link
Copy Markdown
Contributor Author

@henrykironde can you please review and tell me if changes needed??

@ethanwhite
Copy link
Copy Markdown
Member

@abhayrajjais01 - thanks for the contribution, but per our developer guide we want you to engage with us on an issue (and actually hear back from someone; ideally being assigned to the issue) before submitting PRs. @jveitchmichaelis has an opened a WIP PR on this a while ago. You can ask in the original issue if he'd like some help on it and if he says yes then you can proceed.

@ethanwhite ethanwhite closed this Mar 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants