Skip to content

fix(ogc): keep the frame type when a result is empty - #373

Draft
thodson-usgs wants to merge 3 commits into
DOI-USGS:mainfrom
thodson-usgs:fix/empty-result-frame-type
Draft

fix(ogc): keep the frame type when a result is empty#373
thodson-usgs wants to merge 3 commits into
DOI-USGS:mainfrom
thodson-usgs:fix/empty-result-frame-type

Conversation

@thodson-usgs

Copy link
Copy Markdown
Collaborator

The bug

Every OGC getter returns a GeoDataFrame when its filter matches at least one row, and a plain DataFrame when it matches none.

locs = waterdata.get_monitoring_locations(state_name="Wisconsin")   # GeoDataFrame
locs = waterdata.get_monitoring_locations(state_name="Wisconsin",
                                          site_type_code="ZZ")      # DataFrame (!)

So .geometry, .to_crs(), .plot(), and pd.concat across a loop of per-site calls break only on the empty result — the case least likely to be covered by a caller's tests. It is also invisible from df.columns, because the geometry column is still listed.

Why

ogc/shaping.py::_empty_feature_frame exists precisely to prevent this; its docstring calls itself "the single home for this empty-page contract", because a later pd.concat([empty_page, geo_page]) would otherwise downgrade the result and strip geometry/CRS.

But _deal_with_empty runs last, on every getter, 160 lines away in the same file — and built a fresh pd.DataFrame(columns=properties), discarding the guarantee it had just been handed.

The fix

Reindex the frame we were given instead of constructing a new one. That applies the schema's columns while keeping the concrete class — and the geometry dtype with it:

GeoDataFrame().reindex(columns=["monitoring_location_id", "geometry"])
# -> GeoDataFrame, geometry dtype preserved, empty

The plain-pandas path is unchanged: pd.DataFrame().reindex(columns=[...]) is still a pd.DataFrame.

Testing

  • New regression test uses a DataFrame subclass rather than real geopandas, so the invariant is checked even where geopandas is absent (it is not in the [test] extra, only [nldi] and [doc]). Verified to fail against the old line and pass against the new one.
  • 788 passed; mypy --strict, ruff, xenon, complexipy, lint-imports all pass.

Found by the scan in #372, split out because it changes user-visible behavior. Independent of #372 — branches off main.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BTaSm7HmVb94RSJiKW4WAS

Every OGC getter returned a ``GeoDataFrame`` when its filter matched at
least one row and a plain ``DataFrame`` when it matched none.

``_empty_feature_frame`` exists to guarantee an empty page keeps the
concrete class -- its docstring calls itself "the single home for this
empty-page contract", because a later ``pd.concat([empty, geo_page])``
would otherwise strip geometry and CRS. But ``_deal_with_empty`` runs
last, on every getter, 160 lines away in the same file, and built a
fresh ``pd.DataFrame(columns=...)`` -- discarding the guarantee it was
handed.

So ``.geometry``, ``.to_crs()``, ``.plot()`` and ``pd.concat`` across a
loop of per-site calls broke only on the empty result: the case least
likely to be covered by a caller's tests, and invisible from
``df.columns`` because the ``geometry`` column is still listed.

Reindexing the frame we were given applies the schema's columns while
keeping the class -- and the ``geometry`` dtype with it. The plain-pandas
path is unchanged: ``pd.DataFrame().reindex(columns=[...])`` is still a
``pd.DataFrame``.

The regression test uses a ``DataFrame`` subclass rather than real
geopandas, so the invariant is checked even where geopandas is absent
(it is not in the ``[test]`` extra). Verified to fail against the old
line and pass against the new one.

788 passed, mypy --strict clean, all hooks pass.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BTaSm7HmVb94RSJiKW4WAS
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.

1 participant