fix: address review comments from PR #124 (utils cleanup, accessor behavior, tests)#130
Merged
ChrisBarker-NOAA merged 2 commits intoioos:mainfrom Apr 6, 2026
Merged
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
- 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
Contributor
|
This all looks good -- thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
hi @ChrisBarker-NOAA
This PR addresses all review comments raised in and supersedes PR #124. The updates focus on aligning behavior with expected API design, removing deprecated functionality, and improving test clarity and reliability.
Changes
Removed deprecated utils.assign_ugrid_topology shim
Updated codebase to rely on grids.ugrid.assign_ugrid_topology directly
Removed unused imports
Retained np.asarray(poly) in normalize_polygon_x_coords for input consistency
Updated subset_vars to raise ValueError when no grid is recognized
Eliminates silent passthrough behavior and enforces correct usage
Standardized imports to use xarray_subset_grid.utils as a single module
Removed unnecessary bool() wrapping for .all() / .any()
Strengthened mask validation with a deterministic inside-polygon check
Added test for list/tuple polygon coercion
Removed deprecated API test and skip-based test behavior
Removed debug print statements
Updated expectations to reflect ValueError for subset_vars without a grid
Removed zarr__version__ workaround and related skip condition
Retained --online requirement for remote test execution
Testing
Local tests pass for:
tests/test_utils.py
tests/test_accessor.py
tests/test_grids/test_sgrid.py
Online tests remain optional via --online
Notes
This PR replaces #124 rather than modifying it directly
All review comments from Chris have been addressed
No changes to core functionality beyond enforcing correct behavior and removing deprecated APIs
Please tell me if i still need to change anything in this
im really excited about contributing to this project