Skip to content

Type-check compression_level in to_geotiff#3324

Merged
brendancol merged 2 commits into
mainfrom
issue-3321
Jun 14, 2026
Merged

Type-check compression_level in to_geotiff#3324
brendancol merged 2 commits into
mainfrom
issue-3321

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #3321

to_geotiff documents compression_level as int | None, but the
validation only did a range comparison. That left three holes:

  • compression_level=True was accepted as level 1 (bool is an int subclass).
  • A string or list raised a raw TypeError from the lo <= compression_level <= hi comparison instead of a clear message.
  • Codecs with no _LEVEL_RANGES entry (e.g. lzw) skipped the check, so junk passed through and was ignored.

This adds _validate_compression_level_arg in _validation.py, mirroring
the existing _validate_overview_level_arg guard, and calls it in
to_geotiff before the range check. It rejects bool and any non-int
with a TypeError that names the parameter, for every codec. Valid ints
keep the existing out-of-range ValueError behavior. Well-typed ints for
codecs without a level range are still silently ignored, matching how the
rest of the module treats inapplicable-but-well-typed params.

Backend coverage: the guard runs in to_geotiff before backend dispatch,
so it covers numpy, cupy, dask+numpy, and dask+cupy through one path.

Test plan:

  • compression_level=True raises TypeError
  • '9', [9], 9.0, object() raise TypeError
  • non-int rejected for lzw (no level range)
  • valid int still writes
  • out-of-range int still raises ValueError
  • existing writer + codec-roundtrip suites pass (429 + streaming)

@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 14, 2026

@brendancol brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PR Review: Type-check compression_level in to_geotiff

Blockers

None.

Suggestions

None.

Nits

  • _validation.py / eager.py: the new _validate_compression_level_arg is wired into to_geotiff only. The second range check in _write_vrt_tiled (eager.py ~1273) and the GPU writer's own range check (gpu.py ~395) still have no type guard. Both are internal and only reachable through to_geotiff, which now validates first, so there is no live bug here. A one-line comment at those two sites noting the type guard runs upstream would save a future direct caller some confusion. Not blocking.

What looks good

  • The new helper matches _validate_overview_level_arg: same None passthrough, same (int, np.integer) plus not bool check, same TypeError message shape. Reads cleanly against the existing precedent.
  • Placement is right. It runs after compression-name validation (so compression is already a known-good string) and before the range check and backend dispatch, so one path covers numpy, cupy, and dask.
  • Tests cover bool, str, list, float, and an arbitrary object, plus the no-level-range codec (lzw), a valid int that still writes, and an out-of-range int that still raises ValueError. Temp files carry the issue number.
  • Keeping the silent-ignore behavior for well-typed-but-inapplicable ints (rather than rejecting them the way max_z_error does) is the minimal change and matches the rest of the module. The PR body says so.

Checklist

  • Algorithm matches reference: n/a (input validation)
  • Backends consistent: yes, single pre-dispatch path
  • NaN handling: n/a
  • Edge cases covered: yes
  • Dask boundaries: n/a
  • No premature materialization: n/a
  • Benchmark: not needed
  • README matrix: n/a (no new function)
  • Docstrings present and accurate: yes

@brendancol brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PR Review (follow-up pass)

The one nit from the first pass is addressed: the internal range checks in _write_vrt_tiled (eager.py) and _write_geotiff_gpu (gpu.py) now carry a one-line note that to_geotiff rejects non-int / bool compression_level upstream (#3321), so those comparisons are type-safe on the normal dispatch path.

No new findings. Comment-only changes since the last pass; the write + codec-roundtrip suites still pass.

Blockers

None.

Suggestions

None.

Nits

None remaining.

@brendancol
brendancol merged commit 5595b2d into main Jun 14, 2026
7 checks passed
@brendancol
brendancol deleted the issue-3321 branch June 25, 2026 12:57
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 compression_level is not type-safe

1 participant