Skip to content

geotiff: reject band=True/False on all read paths (#1786)#1791

Merged
brendancol merged 2 commits into
mainfrom
issue-1786
May 13, 2026
Merged

geotiff: reject band=True/False on all read paths (#1786)#1791
brendancol merged 2 commits into
mainfrom
issue-1786

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Summary

Closes #1786. The non-VRT read paths only range-checked band. Because isinstance(True, int) is True in Python and True < N evaluates as 1 < N, band=True silently read band 1 and band=False read band 0 through read_to_array, _read_cog_http, read_geotiff_dask, and read_geotiff_gpu. The VRT path already rejected bools, so the contract was inconsistent across read paths.

This adds an isinstance(band, bool) guard before the range check in each non-VRT path, mirroring the VRT path's existing rejection. Uses ValueError with the same wording so all four read paths surface the same error.

Test plan

The non-VRT read paths only range-check `band`. Because
`isinstance(True, int)` is True in Python and `True < N` evaluates as
`1 < N`, `band=True` silently read band 1 and `band=False` read band 0
through `read_to_array`, `_read_cog_http`, `read_geotiff_dask`, and
`read_geotiff_gpu`. The VRT path already rejected bools up front, so
the API contract diverged across backends.

Add an `isinstance(band, bool)` guard before the range check in each
non-VRT path. Uses ValueError with the same wording as the existing
VRT rejection so all four read paths surface an identical error.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 13, 2026
@brendancol brendancol requested a review from Copilot May 13, 2026 13:39
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes inconsistent band input validation across GeoTIFF read backends by rejecting band=True/band=False (which otherwise behave like 1/0 in Python) so callers don’t silently read the wrong band.

Changes:

  • Add explicit bool guards for band in non-VRT read paths (read_to_array, HTTP/COG _read_cog_http, read_geotiff_dask, read_geotiff_gpu).
  • Add regression tests asserting band=True/False is rejected across the main entry points.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
xrspatial/geotiff/tests/test_geotiff_band_bool_rejection_1786.py Adds regression coverage for rejecting band=True/False across multiple read entry points.
xrspatial/geotiff/_reader.py Rejects bool band values in read_to_array and _read_cog_http before range checks.
xrspatial/geotiff/__init__.py Rejects bool band values in read_geotiff_dask and read_geotiff_gpu before range checks.
Comments suppressed due to low confidence (3)

xrspatial/geotiff/tests/test_geotiff_band_bool_rejection_1786.py:21

  • os is imported but never used in this test module; this will fail flake8 (unused import). Remove the import or use it.
import importlib.util
import os
import uuid

xrspatial/geotiff/_reader.py:2401

  • Same as the HTTP path: isinstance(band, bool) does not catch NumPy boolean scalars (np.bool_), which can still be accepted and act like 1/0. Consider rejecting np.bool_ here too to keep backend parity with _vrt.read_vrt.
            # Reject ``bool`` before the range check. ``isinstance(True, int)``
            # is True in Python and ``True < ifd_samples`` evaluates as ``1``,
            # so without this guard ``band=True`` silently reads band 1 and
            # ``band=False`` reads band 0. Matches the VRT path's existing
            # rejection so all read paths agree on the contract. See #1786.
            if isinstance(band, bool):
                raise ValueError(
                    f"band must be a non-negative int, got {band!r}")

xrspatial/geotiff/init.py:2983

  • Same issue as read_geotiff_dask: np.bool_(True/False) will bypass isinstance(band, bool) and can still be treated as 1/0. Reject NumPy boolean scalars here as well to keep parity with the VRT path.
        if band is not None:
            # Reject ``bool`` up front; ``isinstance(True, int)`` is True
            # in Python so ``True < ifd_samples`` evaluates without raising
            # and silently reads band 1. Matches the VRT path's rejection
            # so all read paths agree on the contract. See #1786.
            if isinstance(band, bool):
                raise ValueError(
                    f"band must be a non-negative int, got {band!r}")

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +9 to +14

These tests pin every read entry point -- ``read_to_array`` (local
and HTTP), ``open_geotiff``, ``read_geotiff_dask``,
``read_geotiff_gpu`` (when cupy is available), and ``read_vrt`` --
to the same rejection so all four backends agree: ``band`` must be
a non-negative int, never a bool.
Comment thread xrspatial/geotiff/_reader.py Outdated
Comment on lines +1901 to +1905
# Reject ``bool`` up front; ``isinstance(True, int)`` is True in
# Python so ``True < samples_per_pixel`` evaluates without raising
# and silently reads band 1. Matches the VRT path's rejection so
# all read paths agree on the contract. See #1786.
if isinstance(band, bool):
Comment thread xrspatial/geotiff/__init__.py Outdated
Comment on lines +2285 to +2289
# Reject ``bool`` up front; ``isinstance(True, int)`` is True in
# Python so ``True < n_bands`` evaluates without raising and
# silently reads band 1. Matches the VRT path's rejection so
# all read paths agree on the contract. See #1786.
if isinstance(band, bool):
Address Copilot review feedback on PR #1791:

The bool-rejection guards added in #1786 used ``isinstance(band, bool)``,
which does not catch ``np.bool_`` since ``np.bool_`` is not a subclass
of Python's ``bool``. ``np.bool_(True)`` therefore bypassed the guard
and was then treated as ``1`` by the range check (and ``np.bool_(False)``
as ``0``), silently reading the wrong band -- the exact behaviour the
parent PR was meant to close on every read path.

The VRT path already rejects ``np.bool_`` because its check is
``not isinstance(band, (int, np.integer))`` and ``np.bool_`` is not an
instance of either. Widen the non-VRT guards to
``isinstance(band, (bool, np.bool_))`` so all four read backends
(``read_to_array``, ``_read_cog_http``, ``read_geotiff_dask``,
``read_geotiff_gpu``) agree with the VRT contract.

Also drop an unused ``import os`` from the regression test module.

Tests cover ``np.bool_(True)`` and ``np.bool_(False)`` on every entry
point, including ``read_vrt`` to pin its existing rejection.
@brendancol brendancol merged commit 39fa573 into main May 13, 2026
11 checks passed
brendancol added a commit that referenced this pull request May 13, 2026
PR #1791's review flagged that the regression file only tested local
read paths even though the original fix also touched `_read_cog_http`.
This commit adds four HTTP tests: `band=True`, `band=False`,
`band=np.bool_(True)` all raise `ValueError`, and `band=0` still works.
Uses the same loopback `TCPServer` pattern as
`test_http_band_validation_1695.py`.
brendancol added a commit that referenced this pull request May 13, 2026
Resolves conflict in xrspatial/geotiff/__init__.py: keeps the
`_read_vrt_dask` dispatch hook from the PR branch. All other
geotiff changes from main (#1791, #1793, #1801, #1802, #1803, #1804,
#1805, #1806) were already integrated into the working tree by the
prior 7329dd9 commit; this merge just records the parent so git
recognises the reconciliation.
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.

geotiff: non-VRT read paths accept band=True / band=False

2 participants