Add SGRID Xarray accessor#2638
Conversation
Adds an xarray accessor for SGRID focussing on `metadata` and `rename()`. Removes old functions which handled this (i.e., get_grid_topology, parse_grid_attrs, and parcels.sgrid.rename(ds,...)), and updates all references
Alongside helpers get_n_nodes and get_n_faces
Relax requirement for slice objects in isel
Refactor to avoid calculating the index verbatim. Define NONE and BOTH isel indexing only for contiguous regions. Add more hypothesis testing properties: - P1: consistency invariant (assert_metadata_ds_consistency after any valid isel) - P2: data correctness (values match direct xarray isel) - P3: specification symmetry (isel by node dim == isel by derived face dim) Co-authored-by: Claude <noreply@anthropic.com>
for more information, see https://pre-commit.ci
VeckoTheGecko
force-pushed
the
push-kupxytsrkxzn
branch
from
4f3130f to
8e8910a
Compare
erikvansebille
left a comment
erikvansebille
left a comment
There was a problem hiding this comment.
Looks good; just a few small comments and questions below
| return grid_da | ||
|
|
||
| def isel(self, indexers: Mapping[str, Any] | None = None, **indexers_kwargs): | ||
| """TODO: Docstring""" |
There was a problem hiding this comment.
Something to still do now? Or in a next PR?
There was a problem hiding this comment.
Thanks for the ping - I think its good to do now. Will implement before merge
| f"Due to dataset padding of {padding!r}, expected face dimension {face} to actually be size {expected_n_faces}." | ||
| ) | ||
|
|
||
| # TODO: Also check on coordinates |
There was a problem hiding this comment.
For a next PR, I assume?
| ) | ||
| except Exception as e: | ||
| raise SGridParsingException(f"Failed to parse Grid3DMetadata from {attrs=!r}") from e | ||
|
|
||
| def to_attrs(self) -> dict[str, str | int]: | ||
| d = dict( |
There was a problem hiding this comment.
Just for my own understanding, why has this changed from using dict() to {}? Why would the new approach be better?
There was a problem hiding this comment.
Actually, this change was unnecessary. I thought it would be needed for typing, but it works fine the other way too
| K = TypeVar("K") | ||
| V = TypeVar("V") |
There was a problem hiding this comment.
What's happening here? What is this for?
There was a problem hiding this comment.
These are generic type variables https://mypy.readthedocs.io/en/stable/generics.html
They allow you to describe (via typing) a relationship between the input and output types of a function, without providing concrete types.
e.g.,
T = TypeVar("T")
def make_list(obj: T) -> list[T]:
# make a list of length 1 from provided object
# works on any object type. Returns a list with objects of same type as input
return [obj]In our case here we define type variables K and V (for "key" and "value") for
def invert_non_unique_mapping(d: Mapping[K, V]) -> Mapping[V, list[K]]:
inv_map: dict[V, list[K]] = {}
for k, v in d.items():
inv_map[v] = inv_map.get(v, []) + [k]
return inv_map2 participants
Description
As I work to improve our performance, I would like to do significant work on our software architecture to make things easier to read, maintain, and refactor.
And to help with this re-organisation - I think it would better if we rely more on SGRID-based fieldset creation in our test suite. This will mean that refactoring changes down the line will require little to no updates to the test suite, will result in less setup code in the test suite, and improve readability.
This PR adds an SGRID Xarray dataset accessor to help inspect, and manipulate Xarray SGRID datasets.
Namely:
ds.sgrid.metadataproviding direct access to parsed SGRID metadatads.sgrid.renameallowing the renaming of dataset dimensions alongside the renaming of associated metadata (replaces the oldparcels.sgrid.renamefunction)ds.sgrid.iselselection of spatial dimensions (either by node or by face) which in conjunction selects the other dimension. Allows for the easy subsetting of regions and downsampling of data.As we now have a bunch of xr.Dataset-SGRID logic - this PR also restructures the sgrid to a subpackage within parcels.
Checklist
mainfor normal development,v3-supportfor v3 support)AI Disclosure