Skip to content

contours: keep CRS on empty geopandas result (#2700)#2708

Open
brendancol wants to merge 4 commits into
mainfrom
deep-sweep-metadata-contour-2026-05-29
Open

contours: keep CRS on empty geopandas result (#2700)#2708
brendancol wants to merge 4 commits into
mainfrom
deep-sweep-metadata-contour-2026-05-29

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2700

contours(agg, return_type="geopandas") crashed when the input raster had a crs attribute but the requested levels produced no contour lines (flat raster, levels outside the data range, etc.). _to_geopandas() called gpd.GeoDataFrame(records, crs=crs) with an empty records list, and geopandas rejects a CRS on a frame that has no geometry column.

Changes:

  • _to_geopandas() builds an empty frame with an explicit geometry column when there are no records, so the input CRS still attaches and the result is a well-formed (empty) GeoDataFrame.
  • The all-NaN early-return path now passes the input CRS instead of None, so it no longer silently drops georeferencing.

Backend coverage: the geopandas conversion runs on the host after tracing, so this path is identical for numpy, cupy, dask+numpy, and dask+cupy. No backend-specific code changed.

Test plan:

  • New: empty result with a CRS returns an empty GeoDataFrame carrying the CRS (no crash)
  • New: all-NaN input with auto levels keeps the CRS
  • New: populated result still carries the CRS
  • New: empty result with no input CRS returns an empty frame without error
  • pytest xrspatial/tests/test_contour.py -> 28 passed

_to_geopandas() crashed with 'Assigning CRS to a GeoDataFrame without a
geometry column is not supported' when the result set was empty and the
input had a crs attr. Build an empty frame with an explicit geometry
column so the CRS still attaches. Also propagate the input CRS on the
all-NaN early-return path instead of dropping it.
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: contours: keep CRS on empty geopandas result (#2700)

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

  • xrspatial/contour.py:551 - crs is passed twice in the empty branch: once to gpd.GeoSeries([], crs=crs) and once to the outer GeoDataFrame(..., crs=crs). The outer one sets the frame CRS, so the inner one is redundant. It is harmless and guarantees the geometry column itself carries the CRS, so dropping it is optional.
  • The empty-frame level column comes from [], so its dtype is object/float rather than matching the populated path. This only matters if a caller concatenates an empty result with a populated one and checks dtypes. Low impact.

What looks good

  • The fix targets the exact failure: an empty records list plus a non-None CRS. Building the frame with an explicit geometry column is the right way to let geopandas attach the CRS.
  • The all-NaN early-return now forwards the input CRS instead of None, which closes the silent-drop path.
  • Four new tests cover the populated, empty-with-CRS, all-NaN, and empty-without-CRS cases. The empty-with-CRS test names the regression (#2700).
  • The change is surgical, with no unrelated refactoring.

Checklist

  • Algorithm matches reference: n/a (CRS propagation fix)
  • All implemented backends consistent: yes, conversion is host-side after tracing
  • NaN handling correct: yes, all-NaN path verified by test
  • Edge cases covered by tests: yes
  • Dask chunk boundaries: n/a
  • No premature materialization: yes
  • Benchmark: not needed
  • README feature matrix: not needed
  • Docstrings present and accurate: yes, no API change

@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 29, 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.

Follow-up review (after 5d46d52)

Addressed the first nit: dropped the redundant crs=crs on the inner gpd.GeoSeries([]). The outer GeoDataFrame(..., crs=crs) still sets the frame CRS, and test_geopandas_empty_result_keeps_crs confirms the empty frame carries it. All 28 contour tests pass.

The second nit (empty-frame level column dtype) is left as is. Forcing a dtype on a zero-row column adds noise for an edge case that only matters under cross-frame concatenation with dtype checks, which the library does not do here. Recording it rather than fixing.

No remaining blockers or suggestions.

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.

contours(return_type="geopandas") raises ValueError on empty result with a CRS

1 participant