Skip to content

Guard the datashader import in the rasterizer benchmark#3560

Merged
brendancol merged 2 commits into
mainfrom
issue-3452
Jun 27, 2026
Merged

Guard the datashader import in the rasterizer benchmark#3560
brendancol merged 2 commits into
mainfrom
issue-3452

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #3452.

benchmarks/rasterizer_benchmarks.py keeps datashader as a deliberate A/B comparison, so it stays referenced. But once datashader is no longer installed by default, the unconditional import datashader as ds made the whole script fail to run. This wraps that import (plus spatialpandas, which only feeds the datashader path) in a try/except.

  • With datashader present, HAS_DATASHADER is True and the full comparison runs unchanged.
  • With it absent, HAS_DATASHADER is False, the polygon path skips the datashader timing run, and the datashader cells show -- (same as how the cupy column already handles a missing backend).

Backend coverage: not applicable, this is a benchmark script rather than a spatial op.

Test plan:

  • With datashader installed: confirmed HAS_DATASHADER is True and datashader takes part in both the timing and consistency runs.
  • With datashader and spatialpandas blocked at import: confirmed HAS_DATASHADER is False, the script imports, the xrspatial/geocube/rasterio runners and consistency checks still work, and the markdown report builds.
  • py_compile passes.
  • No packaged unit test: the standalone benchmark scripts have no pytest coverage, and importing this one pulls in benchmark-only deps (geocube, spatialpandas) that the CI test env does not have.

https://claude.ai/code/session_01TF3CsJ7nee9agjajEWLw5d

Wrap the datashader (and spatialpandas, which only feeds the datashader
path) imports in a try/except so the benchmark runs whether or not
datashader is installed. The datashader column is skipped when absent and
included when present.

Claude-Session: https://claude.ai/code/session_01TF3CsJ7nee9agjajEWLw5d

@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: Guard the datashader import in the rasterizer benchmark

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

  • benchmarks/rasterizer_benchmarks.py (generate_markdown): when datashader is absent, the polygon timing and ratio tables still print a "datashader" column header filled with --. The issue wording says "skip the datashader column". Dropping the column outright for polygons would mean threading HAS_DATASHADER into generate_markdown. The -- fallback matches how the cupy column already handles a missing backend, so I read this as a reasonable consistency choice rather than a defect. Flagging it so the maintainer can decide.
  • benchmarks/rasterizer_benchmarks.py:39: the guard catches only ImportError. If datashader is installed but breaks on import for some other reason, that will propagate. Standard practice and fine; noting for completeness.

What looks good

  • spatialpandas and datashader are folded into one guard. That is correct, since spatialpandas only feeds the datashader path, so there is no half-guarded state.
  • Every reference to ds. and spatialpandas (the run_datashader body, the spd_gdf build) sits behind use_datashader, which now requires HAS_DATASHADER. No NameError path when datashader is missing.
  • use_datashader = kind == "polygon" and HAS_DATASHADER is the minimal correct gate.

Checklist

  • Algorithm matches reference/paper: n/a (benchmark infra)
  • All implemented backends produce consistent results: n/a
  • NaN handling is correct: n/a
  • Edge cases covered: both datashader-present and datashader-absent paths verified
  • Dask chunk boundaries handled correctly: n/a
  • No premature materialization or unnecessary copies: yes
  • Benchmark exists or not needed: this is the benchmark
  • README feature matrix updated: not applicable, no new function
  • Docstrings present and accurate: the guard carries an explanatory comment

@brendancol brendancol merged commit 2b48c7b into main Jun 27, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Guard the datashader import in the rasterizer benchmark

1 participant