Skip to content

geotiff: reject non-string compression at the to_geotiff boundary (#2975)#2977

Merged
brendancol merged 1 commit into
mainfrom
issue-2975
Jun 5, 2026
Merged

geotiff: reject non-string compression at the to_geotiff boundary (#2975)#2977
brendancol merged 1 commit into
mainfrom
issue-2975

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2975

to_geotiff raised a raw AttributeError: 'int' object has no attribute 'lower' when compression was a non-string (say an int) and compression_level was set. The public wrapper validated compression names only inside an if isinstance(compression, str): block, so a non-string skipped validation and later hit compression.lower() in the compression_level range check.

  • Add a type guard at the public boundary in eager.py that rejects a non-string, non-None compression with a TypeError, matching the low-level writer's existing guard in _writer.py.
  • The string-name ValueError path and the None exemption 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:

  • New regression tests assert to_geotiff(arr, path, compression=<non-str>, compression_level=1) and the no-level variant raise TypeError instead of AttributeError, parametrized over int/float/list/object.
  • A positive test confirms the default, the 'none' string, and a string + compression_level combo still write.
  • Full write/test_basic.py passes (289 tests); flake8 clean on changed files.

)

A non-string compression skipped the wrapper's name-validation block and
later hit compression.lower() during compression_level validation,
leaking a raw AttributeError. Reject the bad type up front with the same
TypeError shape the low-level writer in _writer.py already uses.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 5, 2026
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

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 exempts None (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 downstream AttributeError from _compression_tag (_encode.py:311), because the signature types compression as str (default 'zstd') and None was never a supported value here. That is pre-existing and out of scope for this PR. I'm flagging it only so None isn't read as a value the new guard now accepts. A follow-up could either treat None as 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 three compression_level range checks), so no non-string can reach .lower().
  • The TypeError type 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

@brendancol brendancol merged commit 80ef887 into main Jun 5, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance PR touches performance-sensitive code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

to_geotiff leaks a raw AttributeError for non-string compression when compression_level is set

1 participant