geotiff: reject non-string compression at the to_geotiff boundary (#2975)#2977
Merged
Conversation
brendancol
commented
Jun 5, 2026
Contributor
Author
brendancol
left a comment
There was a problem hiding this comment.
PR Review: reject non-string compression at the to_geotiff boundary (#2975)
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
None.
Nits (optional improvements)
eager.py:499-- The guard exemptsNone(and compression is not None) to mirror the low-level writer's guard, which is the right call for keeping the two shapes aligned. One thing to note:to_geotiff(arr, path, compression=None)still leaks a downstreamAttributeErrorfrom_compression_tag(_encode.py:311), because the signature typescompressionasstr(default'zstd') andNonewas never a supported value here. That is pre-existing and out of scope for this PR. I'm flagging it only soNoneisn't read as a value the new guard now accepts. A follow-up could either treatNoneas an alias for'none'or reject it at the same boundary.
What looks good
- The guard sits before every
compression.lower()call (the name check, the JPEG and experimental gates, and all threecompression_levelrange checks), so no non-string can reach.lower(). - The
TypeErrortype and message match the low-level writer's guard in_writer.py:215, so the public and private boundaries stay consistent. - Tests cover the reported case plus int/float/list/object, with and without
compression_level, and a positive test confirms the default, the'none'string, and a string + level combo still write. Temp files carry the issue number. - Validation runs in the shared wrapper before backend dispatch, so the fix applies to numpy, cupy, dask+numpy, and dask+cupy without per-backend changes.
Checklist
- Algorithm matches reference/paper: n/a (validation fix)
- All implemented backends produce consistent results: yes (pre-dispatch validation)
- NaN handling is correct: n/a
- Edge cases covered by tests: yes (int/float/list/object, level/no-level, valid codecs)
- Dask chunk boundaries handled correctly: n/a
- No premature materialization or unnecessary copies: yes
- Benchmark exists or is not needed: not needed
- README feature matrix updated: not needed (no new API)
- Docstrings present and accurate: unchanged; compression already documented as str
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 #2975
to_geotiffraised a rawAttributeError: 'int' object has no attribute 'lower'whencompressionwas a non-string (say an int) andcompression_levelwas set. The public wrapper validated compression names only inside anif isinstance(compression, str):block, so a non-string skipped validation and later hitcompression.lower()in thecompression_levelrange check.eager.pythat rejects a non-string, non-Nonecompressionwith aTypeError, matching the low-level writer's existing guard in_writer.py.ValueErrorpath and theNoneexemption are unchanged.Backends: validation happens in the shared public wrapper before backend dispatch, so this covers numpy, cupy, dask+numpy, and dask+cupy.
Test plan:
to_geotiff(arr, path, compression=<non-str>, compression_level=1)and the no-level variant raiseTypeErrorinstead ofAttributeError, parametrized over int/float/list/object.'none'string, and a string +compression_levelcombo still write.write/test_basic.pypasses (289 tests); flake8 clean on changed files.