fix: check self._grid instead of self._ds in data_vars property#121
fix: check self._grid instead of self._ds in data_vars property#121nishantharkut wants to merge 2 commits intoioos:mainfrom
Conversation
Check self._grid before delegating in the accessor data_vars property so unsupported datasets return an empty set instead of calling through a missing grid implementation. Add a focused regression test for the no-grid path, and align nearby accessor return annotations with actual return types to clear return-type diagnostics.
There was a problem hiding this comment.
Pull request overview
This PR fixes the GridDatasetAccessor.data_vars guard so it doesn’t attempt to delegate through self._grid when no grid implementation is recognized, and adds a regression test for the no-grid scenario.
Changes:
- Fix
data_varsto checkself._grid(notself._ds) before delegating to the grid implementation. - Add a regression test asserting
data_varsreturns an empty set when no grid type is recognized (and a warning is emitted). - Update nearby accessor return annotations to better reflect actual return types.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
xarray_subset_grid/accessor.py |
Fixes no-grid guard in data_vars and updates accessor typing for coords/grid_vars. |
tests/test_accessor.py |
Adds a focused regression test for the no-grid accessor case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
xarray_subset_grid/accessor.py
Outdated
| from xarray.core.coordinates import DatasetCoordinates | ||
|
|
xarray_subset_grid/accessor.py
Outdated
| def grid_vars(self) -> set[str] | list[str]: | ||
| """List of grid variables. |
There was a problem hiding this comment.
I think the type annotation suggestion is good -- let's make it always a set
but is this line reqwuired?
set(self._grid.grid_vars(self._ds))
probably better to make sure that _grid._grid_vars is already a set.
- Move DatasetCoordinates import behind TYPE_CHECKING to avoid runtime dependency on private xarray.core module - Revert grid_vars annotation to set[str] to match actual return types
|
Sorry I haven't had time to review these -- this looks good at a glance, but I do want to look closer before merging. A note about type hints -- they are in there, but I'm not very experienced with them. But as a philosophy, I prefer type hints not to be too restrictive -- e.g. don't type it a s |
|
Thanks for the review ! I made the second commit (bbfb6ab) that reverts grid_vars back to set[str] and moves the DatasetCoordinates import behind TYPE_CHECKING which are both based on the Copilot suggestions. The only annotation change remaining is coords, which was returning DatasetCoordinates but annotated as set[str]. |
Summary
This PR fixes the accessor guard used by data_vars when no grid implementation is recognized for a dataset.
Problem
The data_vars property checked self._ds, which is always present after accessor initialization. In no-grid cases, that guard still allowed a call through self._grid, which can be None.
Changes
Validation