GridGeoSampler: change stride of last patch to sample entire ROI #630

remtav · 2022-06-28T18:36:35Z

This PR changes the way in which GridGeoSampler samples patches from each tile if the tile size is not a multiple of the stride. We want to cover the entire tile, requiring us to adjust the stride of the final row/column.

This also changes the number of patches returned from each tile. Let $i$ be the size of the input tile. Let $k$ be the requested size of the output patch. Let $s$ be the requested stride. Let $o$ be the number of output rows/columns sampled from each tile.

Before

$$ o = \left\lfloor \frac{i - k}{s} \right\rfloor + 1 $$

After

$$ o = \left\lceil \frac{i - k}{s} \right\rceil + 1 $$

Reboot of #448
Fixes #431

…l and issue warning

…artial patch (to be padded) test_single.py: add tests for multiple limit cases (see issue microsoft#448)

remtav · 2022-06-28T18:38:17Z

@adamjstewart this is ready for review. I added more tests than necessary. I can remove some if you'd prefer.
Also, I'm not sure how to reproduce the problem you mentionned when two tiles that have a common edge face create a sample with empty area. Tips to implement this tests are welcome.
Currently, see TODO

remtav · 2022-06-28T19:23:33Z

It seems like a test with the NAIP dataset is failing, but on Windows, the tests passes. Can you try on Ubuntu @adamjstewart ? I'll be back home thursday to test on Ubuntu locally if needed.

remtav · 2022-06-29T16:35:23Z

@adamjstewart I added you as collaborator on my fork if you'd like to push some modifications. I'll be in and out until next week

calebrob6 · 2022-06-30T17:07:42Z

@remtav, can you add me as well?

calebrob6 · 2022-07-02T04:28:00Z

Okay so I just played with this for awhile, the NAIPChesapeakeDataModule has stride hardcoded as 128 which I think makes the 2nd box generated by the sampler jump out of the dataset bounds. The test data is 218x128 and strides of < 64 seem to work.

adamjstewart · 2022-07-09T00:50:47Z

Wait a minute. Do we want to sample beyond the ROI?? What if I have a geospatial train/val/test split and the sampler goes beyond the ROI of train into val/test? Wouldn't that be bad?

calebrob6 · 2022-07-09T17:30:20Z

An alternative behavior to make sure we sample all space within the ROI without sampling beyond is to force an additional overlapping box along the right and bottom sides of the ROI.

adamjstewart · 2022-07-09T18:41:21Z

@calebrob6 which of these possible solutions does that correspond to? #448 (comment)

calebrob6 · 2022-07-09T19:34:02Z

Adjust the stride of the last patch only -- e.g. of the logic is here https://github.com/microsoft/poultry-cafos/blob/main/cafo/data/TileDatasets.py#L65

remtav · 2022-07-10T13:24:30Z

Adjust the stride of the last patch only -- e.g. of the logic is here https://github.com/microsoft/poultry-cafos/blob/main/cafo/data/TileDatasets.py#L65

That's also the solution I had implemented in my first solution (draft):
cb554c6

adamjstewart · 2022-08-22T06:18:24Z

Trying to release 0.3.1 in approximately 1 week. @remtav can you revert back to your original solution and we can iterate from there? If you're busy, I can take over from here. I think we've agreed on what the correct behavior should be, which was the hardest part. Note that #736 may also affect the same lines of code.

…into samplers/gridgeosampler_bounds # Conflicts: # torchgeo/samplers/single.py

remtav · 2022-08-25T20:19:26Z

Trying to release 0.3.1 in approximately 1 week. @remtav can you revert back to your original solution and we can iterate from there? If you're busy, I can take over from here. I think we've agreed on what the correct behavior should be, which was the hardest part. Note that #736 may also affect the same lines of code.

Sure, I can revert and build on top of initial solution (adjust stride of last patch) with unit tests and formatting clean up. I'll push at the latest on monday.

…r row/col and issue warning" This reverts commit cb554c6

adamjstewart

Still need to check to make sure the rounding is right, will compare to #736 later.

torchgeo/samplers/single.py

remtav · 2022-08-29T20:45:43Z

Ready for review @adamjstewart
As for rounding issues, I've favored using /, then math.ceil, rather than the original // and +1 approach (which doesn't seem right when ROI and grid sampling area are equal anyways):

Old:
int((bounds.maxy - bounds.miny - self.size[0]) // self.stride[0]) + 1
New:
math.ceil((bounds.maxy - bounds.miny - self.size[0] + self.stride[0]) / self.stride[0]

Hopefully that does it, but please double check. There might also be small conflicts with #736.

adamjstewart

Code looks correct for all situations I can think of. Just a few remaining questions/suggestions before this is ready to merge.

tests/samplers/test_single.py

torchgeo/samplers/single.py

tests/samplers/test_single.py

adamjstewart · 2022-09-03T04:06:59Z

Thanks for the fix @remtav! Sorry this took so long to merge.

…rosoft#630) * Adjust minx/miny with a smaller stride for the last sample per row/col and issue warning * style and mypy fixes * black test fix * Adjust minx/miny with a smaller stride for the last sample per row/col and issue warning * style and mypy fixes * black test fix * single.py: adapt gridgeosampler to sample beyond limit of ROI for a partial patch (to be padded) test_single.py: add tests for multiple limit cases (see issue microsoft#448) * format for black and flake8 * format for black and flake8 * once again, format for black and flake8 * Revert "Adjust minx/miny with a smaller stride for the last sample per row/col and issue warning" This reverts commit cb554c6 * adapt unit tests, remove warnings * flake8: remove warnings import * Address some comments * Simplify computation of # rows/cols * Document this new feature * Fix size of ceiling symbol * Simplify tests Co-authored-by: Adam J. Stewart <[email protected]>

* Adjust minx/miny with a smaller stride for the last sample per row/col and issue warning * style and mypy fixes * black test fix * Adjust minx/miny with a smaller stride for the last sample per row/col and issue warning * style and mypy fixes * black test fix * single.py: adapt gridgeosampler to sample beyond limit of ROI for a partial patch (to be padded) test_single.py: add tests for multiple limit cases (see issue #448) * format for black and flake8 * format for black and flake8 * once again, format for black and flake8 * Revert "Adjust minx/miny with a smaller stride for the last sample per row/col and issue warning" This reverts commit cb554c6 * adapt unit tests, remove warnings * flake8: remove warnings import * Address some comments * Simplify computation of # rows/cols * Document this new feature * Fix size of ceiling symbol * Simplify tests Co-authored-by: Adam J. Stewart <[email protected]>

…rosoft#630) * Adjust minx/miny with a smaller stride for the last sample per row/col and issue warning * style and mypy fixes * black test fix * Adjust minx/miny with a smaller stride for the last sample per row/col and issue warning * style and mypy fixes * black test fix * single.py: adapt gridgeosampler to sample beyond limit of ROI for a partial patch (to be padded) test_single.py: add tests for multiple limit cases (see issue microsoft#448) * format for black and flake8 * format for black and flake8 * once again, format for black and flake8 * Revert "Adjust minx/miny with a smaller stride for the last sample per row/col and issue warning" This reverts commit cb554c6 * adapt unit tests, remove warnings * flake8: remove warnings import * Address some comments * Simplify computation of # rows/cols * Document this new feature * Fix size of ceiling symbol * Simplify tests Co-authored-by: Adam J. Stewart <[email protected]>

remtav added 8 commits March 2, 2022 11:28

Adjust minx/miny with a smaller stride for the last sample per row/co…

ad093fb

…l and issue warning

style and mypy fixes

855fee1

black test fix

eb33fe0

Adjust minx/miny with a smaller stride for the last sample per row/co…

cb554c6

…l and issue warning

style and mypy fixes

1a0236c

black test fix

bfadf76

single.py: adapt gridgeosampler to sample beyond limit of ROI for a p…

6942b5f

…artial patch (to be padded) test_single.py: add tests for multiple limit cases (see issue microsoft#448)

format for black and flake8

138855b

github-actions bot added samplers Samplers for indexing datasets testing Continuous integration testing labels Jun 28, 2022

remtav added 2 commits June 28, 2022 14:39

format for black and flake8

37d6055

once again, format for black and flake8

b0ab3fa

calebrob6 closed this Jul 2, 2022

calebrob6 reopened this Jul 2, 2022

adamjstewart added this to the 0.3.0 milestone Jul 9, 2022

adamjstewart modified the milestones: 0.3.0, 0.3.1 Jul 9, 2022

remtav and others added 2 commits August 25, 2022 16:14

Merge branch 'microsoft:main' into samplers/gridgeosampler_bounds

45b3490

Merge remote-tracking branch 'origin/samplers/gridgeosampler_bounds' …

720cf5b

…into samplers/gridgeosampler_bounds # Conflicts: # torchgeo/samplers/single.py

Revert "Adjust minx/miny with a smaller stride for the last sample pe…

af5a3d1

…r row/col and issue warning" This reverts commit cb554c6

remtav dismissed a stale review via af5a3d1 August 29, 2022 16:12

Merge branch 'microsoft:main' into samplers/gridgeosampler_bounds

e588385

adamjstewart reviewed Aug 29, 2022

View reviewed changes

torchgeo/samplers/single.py Outdated Show resolved Hide resolved

torchgeo/samplers/single.py Outdated Show resolved Hide resolved

adapt unit tests, remove warnings

0e61b1d

remtav and others added 2 commits August 30, 2022 09:45

flake8: remove warnings import

6c623d8

Merge branch 'main' into samplers/gridgeosampler_bounds

fd9b69a

remtav requested a review from adamjstewart August 30, 2022 20:16

adamjstewart changed the title ~~GridGeoSampler: Sample beyond roi limit if needed~~ GridGeoSampler: change stride of last patch to sample entire ROI Sep 1, 2022

adamjstewart reviewed Sep 2, 2022

View reviewed changes

adamjstewart added 6 commits September 2, 2022 19:56

Merge branch 'main' into samplers/gridgeosampler_bounds

13daca1

Address some comments

9d68d1e

Simplify computation of # rows/cols

8660f28

Document this new feature

9536970

Fix size of ceiling symbol

32d877e

Simplify tests

a741129

adamjstewart approved these changes Sep 3, 2022

View reviewed changes

adamjstewart enabled auto-merge (squash) September 3, 2022 04:06

adamjstewart merged commit 7fa0fd4 into microsoft:main Sep 3, 2022

adamjstewart deleted the samplers/gridgeosampler_bounds branch September 3, 2022 04:11

adamjstewart mentioned this pull request Sep 3, 2022

Random GeoSamplers: add default length #755

Merged

This was referenced May 12, 2023

Concerns with GridGeoSampler for evaluation #1245

Closed

GridGeoSampler: don't change stride of last patch #1329

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GridGeoSampler: change stride of last patch to sample entire ROI #630

GridGeoSampler: change stride of last patch to sample entire ROI #630

remtav commented Jun 28, 2022 •

edited by adamjstewart

Loading

remtav commented Jun 28, 2022

remtav commented Jun 28, 2022 •

edited

Loading

remtav commented Jun 29, 2022 •

edited

Loading

calebrob6 commented Jun 30, 2022

calebrob6 commented Jul 2, 2022

adamjstewart commented Jul 9, 2022

calebrob6 commented Jul 9, 2022

adamjstewart commented Jul 9, 2022

calebrob6 commented Jul 9, 2022 •

edited

Loading

remtav commented Jul 10, 2022 •

edited

Loading

adamjstewart commented Aug 22, 2022

remtav commented Aug 25, 2022 •

edited

Loading

adamjstewart left a comment

remtav commented Aug 29, 2022 •

edited

Loading

adamjstewart left a comment

adamjstewart commented Sep 3, 2022

GridGeoSampler: change stride of last patch to sample entire ROI #630

GridGeoSampler: change stride of last patch to sample entire ROI #630

Conversation

remtav commented Jun 28, 2022 • edited by adamjstewart Loading

Before

After

remtav commented Jun 28, 2022

remtav commented Jun 28, 2022 • edited Loading

remtav commented Jun 29, 2022 • edited Loading

calebrob6 commented Jun 30, 2022

calebrob6 commented Jul 2, 2022

adamjstewart commented Jul 9, 2022

calebrob6 commented Jul 9, 2022

adamjstewart commented Jul 9, 2022

calebrob6 commented Jul 9, 2022 • edited Loading

remtav commented Jul 10, 2022 • edited Loading

adamjstewart commented Aug 22, 2022

remtav commented Aug 25, 2022 • edited Loading

adamjstewart left a comment

Choose a reason for hiding this comment

remtav commented Aug 29, 2022 • edited Loading

adamjstewart left a comment

Choose a reason for hiding this comment

adamjstewart commented Sep 3, 2022

remtav commented Jun 28, 2022 •

edited by adamjstewart

Loading

remtav commented Jun 28, 2022 •

edited

Loading

remtav commented Jun 29, 2022 •

edited

Loading

calebrob6 commented Jul 9, 2022 •

edited

Loading

remtav commented Jul 10, 2022 •

edited

Loading

remtav commented Aug 25, 2022 •

edited

Loading

remtav commented Aug 29, 2022 •

edited

Loading