test: cover utils helpers and xsg accessor; fix deprecations#124
test: cover utils helpers and xsg accessor; fix deprecations#124Divyateja2709 wants to merge 1 commit intoioos:mainfrom
Conversation
- Add tests for format_bytes, asdatetime, compute_2d_subset_mask, and utils.assign_ugrid_topology forwarding with DeprecationWarning - Add accessor tests for unknown-grid warning and None subset behavior - Fix warnings.warn argument order in assign_ugrid_topology; correct message - Coerce polygon coords via np.asarray in normalize_polygon_x_coords - Define zarr__version__ when fsspec/zarr import fails (test collection) Made-with: Cursor
ChrisBarker-NOAA
left a comment
ChrisBarker-NOAA
left a comment
There was a problem hiding this comment.
This looks great, thanks!
A couple small comments to addresss, and then I'll merge.
| from xarray_subset_grid.grids.sgrid import _get_location_info_from_topology | ||
|
|
||
| # open dataset as zarr object using fsspec reference file system and xarray | ||
| zarr__version__ = 0 |
There was a problem hiding this comment.
I think we need to check the status of zarr3 and FSpec AWS -- this may be a non_issue now.
| ds = xr.Dataset({"a": (("x",), [1, 2, 3])}) | ||
| with pytest.warns(UserWarning, match="no grid type"): | ||
| out = ds.xsg.subset_vars(["a"]) | ||
| # Without a recognized grid, subset_vars returns the dataset unchanged. |
There was a problem hiding this comment.
Hmm -- this might take a bit of thought -- I think it should simply raise an Error.
There is no use-case for subset_vars without a grid, and having it be do-nothing might be more surprising.
| import xarray as xr | ||
|
|
||
| from tests.conftest import EXAMPLE_DATA | ||
| from xarray_subset_grid import utils as xsg_utils |
There was a problem hiding this comment.
We should either import the whole utils module, or import a bunch of names, but not both.
| assert unit in format_bytes(num) | ||
|
|
||
|
|
||
| def test_asdatetime_none(): |
There was a problem hiding this comment.
thanks! I actually copied that ober from another project and forgot to bring the tests over -- so this is nice.
| poly = np.array([(-75.0, 39.0), (-69.0, 39.0), (-69.0, 45.0), (-75.0, 45.0)]) | ||
| mask = compute_2d_subset_mask(lat_da, lon_da, poly) | ||
| assert mask.dims == ("y", "x") | ||
| assert bool(mask.all()) |
There was a problem hiding this comment.
you don't need the bool() call here -- .all() returns a numpy bool, which works fine. (I think assert probably calls bool() anyway).
| assert bool(mask.all()) | ||
|
|
||
|
|
||
| def test_compute_2d_subset_mask_partial(): |
There was a problem hiding this comment.
it would be nice to have a simple test case that checks that the mask is actually correct.
the actual point-in-polygon code should be tested elswhere, but good to know it's being applied properly -- at least on one example.
|
|
||
| def test_assign_ugrid_topology_utils_deprecation_wrapper(): | ||
| nc = EXAMPLE_DATA / "SFBOFS_subset1.nc" | ||
| if not nc.is_file(): |
There was a problem hiding this comment.
we should either make sure it's there or not -- semi-silently skipping is not good.
| if not nc.is_file(): | ||
| pytest.skip("example NetCDF not present") | ||
| ds = xr.open_dataset(nc) | ||
| with pytest.warns(DeprecationWarning, match="assign_ugrid_topology"): |
There was a problem hiding this comment.
Actually, let's jsut get rid of that Depricated Function -- and make sure that none of the examples use it!
| x (np.array): x-coordinates of the vertices | ||
| poly (np.array): polygon vertices | ||
| """ | ||
| poly = np.asarray(poly) |
There was a problem hiding this comment.
Good idea, thanks!
- Remove deprecated assign_ugrid_topology from utils (use grids.ugrid only) - Raise ValueError from xsg.subset_vars when no grid is recognized - test_utils: single utils import style; stronger mask checks; list polygon test - test_accessor: expect ValueError for subset_vars without grid - test_sgrid: drop zarr>=3 skip for online kerchunk test (per review) Made-with: Cursor
…havior, tests) (#130) * test: cover utils helpers and xsg accessor; fix deprecations - Add tests for format_bytes, asdatetime, compute_2d_subset_mask, and utils.assign_ugrid_topology forwarding with DeprecationWarning - Add accessor tests for unknown-grid warning and None subset behavior - Fix warnings.warn argument order in assign_ugrid_topology; correct message - Coerce polygon coords via np.asarray in normalize_polygon_x_coords - Define zarr__version__ when fsspec/zarr import fails (test collection) Made-with: Cursor * fix: address PR #124 review (subset_vars, utils cleanup, tests) - Remove deprecated assign_ugrid_topology from utils (use grids.ugrid only) - Raise ValueError from xsg.subset_vars when no grid is recognized - test_utils: single utils import style; stronger mask checks; list polygon test - test_accessor: expect ValueError for subset_vars without grid - test_sgrid: drop zarr>=3 skip for online kerchunk test (per review) Made-with: Cursor
|
Add fixed by #130 |
2 participants
This PR improves test coverage and reliability by adding unit tests for utility functions and accessor behavior, along with small fixes to ensure correct warnings and robust handling of inputs.
Changes
Tests
Added
tests/test_utils.py:format_bytesasdatetime(None,datetime,cftime, ISO string)compute_2d_subset_mask(full vs partial coverage)utils.assign_ugrid_topology(deprecation + delegation)Added
tests/test_accessor.py:Tests
ds.xsgbehavior when no grid is recognized:subset_polygonandsubset_bboxreturnNonesubset_varspassthroughhas_vertical_levelsbehaviorUpdated
tests/test_grids/test_sgrid.py:zarr__version__ = 0when optional dependencies are missingNameError)Fixes / Improvements
Deprecation handling
warnings.warn(...)with proper signature andstacklevel=2assign_ugrid_topologycorrectlyInput robustness
normalize_polygon_x_coordsnow coerces inputs usingnp.asarray(poly)Why this matters
Notes
docs/readme-quickstart-fixes)