Fix UgridIndex implementation, remove metaclass trickery#436
Open
rsignell wants to merge 2 commits into
Open
Conversation
…dex refactor Completes the UgridIndex-based architecture where UgridDataArray/UgridDataset return plain xr.DataArray/xr.Dataset with embedded UgridIndex instead of custom wrapper classes. Library fixes: - UgridIndex.equals(): use grid.equals() (value comparison) instead of identity check so binary ops between DataArrays from the same grid work - UgridIndex: add join(), reindex_like() for xarray alignment support - UgridIndex1d._variables_from_dataset(): fix return signature to match (variables, options) tuple expected by from_dataset() - UgridDataset.__new__(): implement grids= path and obj+grids= path - dataarray_accessor: fix to_crs(), to_dataset(), from_structured2d() for new API; call _update_coordinate_attrs after CRS transform - dataset_accessor: fix from_dataset() to restore coord attrs after copy; fix to_crs() and to_dataset() for new API - ugridbase.sel_points(): only add dim_coords when other_dims is non-empty - regridder.py: reorder isinstance checks (UgridDataArray before xr.DataArray), strip UgridIndex from source before regridding structured result - regrid/network.py, unstructured.py: fix obj.grid -> obj.ugrid.grid Test fixes: - Update .grid -> .ugrid.grid, .obj -> direct DataArray usage throughout - Fix scalar comparisons result.min() >= disk.min() to use .item() to avoid MergeError when comparing 0-dim DataArrays from different grids - Fix UgridDataArray constructor calls removing deprecated grid= keyword form - Fix test_ugrid_dataset.py: remove .obj refs, fix multi-topology grid lookup, use .values for cross-grid comparisons Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…checks Per reviewer feedback, the metaclass (_UgridDataArrayMeta / _UgridDatasetMeta) approach is "slightly cursed" and exists only to minimize test changes. This commit removes the metaclasses entirely and makes the design principled: - UgridDataArray and UgridDataset are plain factory classes whose __new__ returns a plain xr.DataArray / xr.Dataset with a UgridIndex attached - Two predicate helpers are added and exported: is_ugrid_dataarray(obj) and is_ugrid_dataset(obj), which check isinstance + _has_ugrid_index - All isinstance(obj, UgridDataArray/UgridDataset) checks in library code are replaced with the predicate helpers (~10 sites) - All isinstance checks in test files are replaced similarly (~100 sites) Also fixes the rename() method in both accessor classes: previously calling xarray's obj.rename() on a UgridIndex-backed object caused errors because xarray tries to rename the index's coordinates (which span dimensions not present after stripping). Fix: call drop_ugrid_index() first, compute to_rename from the stripped object, then re-attach via UgridDataArray/Dataset. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
Contributor
Author
|
@Huite anything for me to do here -- I'm guessing you've been just too busy to look, right? |
Collaborator
|
Hi @rsignell That's right, I've reserved the week of June 15th to take a proper look at this. But I'm feeling fairly hopeful that I'll be able to get things merged in that week! |
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.
Summary
This PR picks up where #373 left off and fixes the remaining broken tests and design issues.
Fixes the known bugs (#373 left TODO):
UgridIndex1d._variables_from_dataset()return signature fixed to(variables, options)tupleUgridDataset(grids=...)andUgridDataset(obj, grids=...)paths implemented inwrap.pyUgridIndex.equals()uses value comparison (grid.equals()) not identity — fixes binary ops between DataArrays derived from same gridUgridIndex.join(),reindex_like()added for xarray alignment supportRemoves metaclass trickery (per reviewer feedback on #435):
_UgridDataArrayMeta/_UgridDatasetMetaentirely — these existed only to makeisinstancepass for plainxr.DataArray, which the reviewer correctly identified as "slightly cursed"is_ugrid_dataarray(obj)andis_ugrid_dataset(obj)predicate helpers, exported from the top-level namespaceisinstance(obj, UgridDataArray/UgridDataset)checks with the new predicates in both library (~10 sites) and tests (~100 sites)Fixes
rename()method:obj.rename()on a UgridIndex-backed object failed because xarray tried to rename index-backing dimensions that no longer existed after strippingdrop_ugrid_index()before renaming, computeto_renamefrom the stripped object, then re-attach via factoryOther fixes:
UgridDataArrayAccessor.to_crs(),to_dataset(),from_structured2d()updated for new index architectureUgridDatasetAccessor.from_dataset()restores coordinate attrs after copy;to_crs()andto_dataset()updatedugridbase.sel_points(): only add dim_coords when indexing multiple grid dims simultaneouslyregrid/: allobj.grid→obj.ugrid.grid,obj.obj→ direct DataArray usage; isinstance check order fixedresult.min() >= disk.min()use.item()to avoid MergeError across different-grid DataArraysTest plan
pytest tests/test_ugrid_dataset.py— all pass (2 pre-existing missingpymetisskips)pytest tests/test_ugrid2d.py— all pass (1 pre-existing missingmapbox_earcutskip)pytest tests/test_regrid/— all 111 passmapbox_earcut,pymetis,pytest_cases)🤖 Generated with Claude Code