RandomGeoSampler: fix performance regression #1968
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adamjstewart
commented
Mar 28, 2024
| height = (bounds.maxy - bounds.miny - t_size[0]) // res | ||
| # May be negative if bounding box is smaller than patch size | ||
| width = (bounds.maxx - bounds.minx - t_size[1]) / res | ||
| height = (bounds.maxy - bounds.miny - t_size[0]) / res |
There was a problem hiding this comment.
This is the width/height in pixel units. It no longer needs to be an integer, float is fine too. We cast to integer elsewhere.
adamjstewart
commented
Mar 28, 2024
|
|
||
| minx = bounds.minx | ||
| miny = bounds.miny | ||
|
|
||
| # random.randrange crashes for inputs <= 0 |
There was a problem hiding this comment.
This is a dead comment, we no longer use random.randrange and the input can no longer be negative
adamjstewart
commented
Mar 28, 2024
|
|
||
| minx = bounds.minx | ||
| miny = bounds.miny | ||
|
|
||
| # random.randrange crashes for inputs <= 0 | ||
| if width > 0: |
There was a problem hiding this comment.
No longer need to guard for negative numbers, it doesn't matter if the sample starts outside the bounds of the image for bounding boxes smaller than the patch size.
adamjstewart
commented
Mar 28, 2024
| if height > 0: | ||
| miny += torch.rand(1).item() * height * res | ||
| # Use an integer multiple of res to avoid resampling | ||
| minx += int(torch.rand(1).item() * width) * res |
There was a problem hiding this comment.
This is the only real important line
5 tasks
adamjstewart
changed the title
|
Evaluation using #1972:
A 60% speedup for RandomGeoSampler when data is preprocessed (same CRS, res, TAP, COG). |
isaaccorley
approved these changes
Apr 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes a bug I introduced all the way back in #477. Thanks @yichiac for noticing this!
We want to try to sample from a pixel-aligned grid whenever possible to avoid resampling in the average case. With the changes in this PR, we should revert back to the behavior we documented in our paper, where preprocessing greatly improves sampling performance. GridGeoSampler was not affected by this bug.
I still want to create an I/O benchmarking script similar to what we used in our original paper to ensure that this indeed works as advertised, so will keep as a draft for now.