geotiff: enforce tile_size positivity in array-level writers (#2997)#3000
Merged
Conversation
_write and _write_streaming used to under-validate tile_size. _write_streaming skipped it entirely, so tile_size=0 hit a bare ZeroDivisionError in the math.ceil(width / tw) layout math; _write only checked positivity under cog=True. Both now run the positivity/type check on any path that consumes tile_size, reusing _validate_tile_size_arg. The multiple-of-16 TIFF spec rule stays at the public to_geotiff boundary: the array-level writers are the lower-level tool, and the in-repo reader plus the existing COG/round-trip tests rely on small spec-noncompliant tiles. Added a require_multiple_of_16 flag (default True) to _validate_tile_size so the public path keeps full enforcement while the private writers opt out of just that rule.
brendancol
commented
Jun 6, 2026
Contributor
Author
brendancol
left a comment
There was a problem hiding this comment.
PR Review: geotiff: enforce tile_size positivity in array-level writers (#2997)
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
- The inner COG positivity guard at
xrspatial/geotiff/_writer.py:560-565is now dead code. The new top-of-function check (if tiled or cog: _validate_tile_size_arg(...)) rejects a non-positivetile_sizebefore the auto-overview block ever runs, and that block only runs whencog=True. So the old guard's narrower message ("for COG overview generation") can't fire anymore. It's pre-existing defense-in-depth and harmless to leave, but the next person to read that block might think COG validation lives there. Either drop it or leave a one-line comment pointing at the top-level gate.
Nits (optional improvements)
- The new comment blocks run long, but they match the comment density of the rest of the file, so no real objection.
What looks good
require_multiple_of_16defaults toTrue, so the publicto_geotiffand the GPU writer keep the exact enforcement they had before. Checked the GPU call site at_writers/gpu.py:365and it uses the default.- Keeping the multiple-of-16 rule at the public boundary instead of shoving it down is the right call, and the PR explains why: the array-level writers and the in-repo reader use small spec-noncompliant tiles, and pushing the rule down broke about a dozen existing COG/round-trip tests.
- Tests cover the cases that matter: 0, negative, and non-int rejected on both
_writeand_write_streaming; small non-multiple-of-16 tiles still write; strip layout ignorestile_size; and a guard test confirms the public path still rejects 17. - The
_write_streaminggate keys ontiledalone, which is correct since there's nocogparameter on that function.
Checklist
- Algorithm matches reference/paper: N/A (validation fix)
- Backends consistent: validation runs before dispatch, so it's backend-independent
- NaN handling: N/A
- Edge cases covered: yes (0, negative, non-int, strip path, valid sizes)
- Dask chunk boundaries: N/A
- No premature materialization: N/A
- Benchmark: not needed
- README matrix: N/A (no new public API)
- Docstrings: yes, the helper docstring documents the new flag
Review flagged the auto-overview tile_size guard as now-unreachable dead code. It is deliberate loop-side defense pinned by test_inner_overview_loop_guard_message_is_pinned (#2311); removing it breaks that contract. Keep the guard and note in the comment that the top-level gate now catches the input first, so the redundancy is by design rather than an oversight.
brendancol
commented
Jun 6, 2026
Contributor
Author
brendancol
left a comment
There was a problem hiding this comment.
Follow-up review (after 1c549a2)
Re-reviewed after the follow-up commit.
Disposition of the prior suggestion
- The inner COG overview guard at
xrspatial/geotiff/_writer.pyis kept, not removed. It turns out an existing test,test_inner_overview_loop_guard_message_is_pinned(added in #2311), deliberately pins that guard's message as loop-side defense, so removing it breaks that contract. The follow-up addresses the "a future reader might be misled" concern the other way: the comment now says outright that the top-levelif tiled or coggate catches the bad input first and the inner guard is redundant by design. That resolves the readability worry without dropping the pinned defense.
Remaining items
- None. No blockers, no open suggestions.
Verification
xrspatial/geotiff/tests/write/plustests/unit/test_input_validation.py: 1223 passed, 1 skipped.
This file contains hidden or 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
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.
Closes #2997
What
The public
to_geotiff()validatestile_size(positive int + multiple of 16) whenevertiledorcogis set, but the array-level private writers didn't all do the same:_write_streaming()skippedtile_sizevalidation, sotile_size=0reachedmath.ceil(width / tw)and raised a bareZeroDivisionError._write()only checked positivity undercog=True; thetiled=True, cog=Falsepath passed the value straight through.Both now run the positivity/type check on any path that consumes
tile_size, reusing the existing_validate_tile_size_arghelper.A note on the multiple-of-16 rule
I did not push the multiple-of-16 TIFF spec rule down to the array-level writers. That rule is enforced at the public
to_geotiffboundary and stays there. The array-level writer is the lower-level tool, and the in-repo reader plus the existing COG and round-trip tests deliberately use small spec-noncompliant tiles (4, 8) to keep test rasters small. Forcing multiple-of-16 at this layer broke about a dozen existing tests. So_validate_tile_sizegained arequire_multiple_of_16flag (defaultTrue) and the private writers opt out of just that rule while keeping the crash-inducing positivity/type checks.Backend coverage
Validation runs before any backend dispatch, so it applies to numpy, cupy, dask+numpy, and dask+cupy alike. The GPU writer keeps full multiple-of-16 enforcement (it is reached through the public-style boundary).
Test plan
_write/_write_streamingrejecttile_size=0, negative, and non-int on the tiled pathcog=Truestill rejects non-positivetile_sizetiled=False) ignorestile_sizeto_geotiffstill enforces multiple-of-16