Skip to content

fix multiply_bounds for odd-sized windows#558

Open
pjreddie wants to merge 2 commits intomasterfrom
fix-multiply-bounds-ceil
Open

fix multiply_bounds for odd-sized windows#558
pjreddie wants to merge 2 commits intomasterfrom
fix-multiply-bounds-ceil

Conversation

@pjreddie
Copy link
Copy Markdown
Contributor

Summary

  • multiply_bounds was floor-dividing each coordinate independently, which made the output size depend on absolute position and raised ValueError for odd dimensions (e.g. 127x127 windows with zoom_offset=-1)
  • switched to ceiling division on width/height so any window size works and output is deterministic
  • added tests for multiply_bounds (even, odd, denom=4, position-independence, numerator path)

Context

hit this materializing a 29k window dataset where all windows were 127x127. the 10m bands materialized fine but all 20m/60m bands failed since 127 isn't divisible by 2 or 4. the old datasets built before the resolution factor validation was added worked fine, new ones don't.

Test plan

  • existing tests pass
  • new multiply_bounds tests cover even division, odd division (127/2=64, 127/4=32), deterministic size across offsets, and numerator path

🤖 Generated with Claude Code

floor-dividing each coordinate independently makes the output size
depend on absolute position, and raises ValueError for odd dimensions
like 127x127. use ceil on width/height instead so any window size works
and output size is deterministic.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@favyen2 favyen2 left a comment

Choose a reason for hiding this comment

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

One issue is that this means that GeoTIFFs at coarser resolutions for these windows won't correspond exactly to the window extent. It may need more thought whether the error message should be more helpful but still raise error or if it should do this ceiling operation; for 127x127 windows one solution is just storing all bands at the window resolution (like 10 m/pixel).

Comment thread rslearn/utils/geometry.py Outdated
Comment thread rslearn/utils/geometry.py Outdated
@favyen2
Copy link
Copy Markdown
Collaborator

favyen2 commented Mar 25, 2026

Hadrien is adding spatial_size option in #564

I wonder if that would address the same issue encountered in this PR?

Basically instead of specifying a factor for the resolution, it supports setting the width/height you want the data to be materialized at. Then the resolution is computed based on the width/height and window extent.

This has the advantage of making sure the materialized GeoTIFFs still correspond exactly to the window extent.

Alternatively for odd-sized windows you could store all the Sentinel-2 bands at 10 m/pixel.

@pjreddie
Copy link
Copy Markdown
Contributor Author

how did this work previously? it used to not be a problem, I've always materialized odd size images because we have one center pixel labelled and we want 63 (or some odd number) of pixels on each side so we can take 64x64 crops and they always have the label pixel in them. this worked until the changes in December/January I think

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@favyen2
Copy link
Copy Markdown
Collaborator

favyen2 commented Mar 27, 2026

I'm not sure, it might've created 32x32 GeoTIFF that goes beyond the window bounds before.

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