-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
no location
variable attribute present in netcdf file written by xugrid
#221
Comments
I must admit I'm a little confused by the introduction of these attributes: they seem fully redundant, because the dimension already associates data with a specific aspect (node, edge, face) of the topology as you mention. From glancing the UGRID conventions docs, they are only mentioned for 3D layered mesh topology and onwards. The UGRID convention pages specifically link to this section: https://ugrid-conventions.github.io/ugrid-conventions/#data-defined-on-unstructured-meshes I think this gives a bit of background:
So if you have e.g. staggered data on a structured grid, you need some additional attribute to indicate the location. For UGRID conventions, this isn't required: nodes, edges, and faces are explicitly represented. However, if you want to be "consistent" in some sense, you might argue that if you are specifying the location for a structured topology, that you can also do for it an unstructured topology -- even though the UGRID conventions do not require it at all. Anyway, my understanding is possibly a bit limited, but this does not really seem part of the UGRID conventions to me. In particular, the redundancy also creates openings for conflicts. Let's say your dimension is I would strongly suggest a single source of truth provided by the dimensions, not these additional attributes for D-Ecoimpact. |
Thanks for this elaborate explanation. Could you share a short snippet of xugrid code on how to quickly match a particular variable to node/face/edge? I looked around a bit but could not find what I was looking for real quick. If it is too much effort, I will dive a bit deeper. |
Regarding this point:
Xugrid infers the dimension based on the connectivity variables and the coordinates (so e.g. edge_node_connectivity should have dimensions (edge_dim, 2); edge_x should have (edge_dim,). Worth nothing that there's already room for conflicts here: somebody can specify This function mostly takes care of it: xugrid/xugrid/ugrid/conventions.py Lines 250 to 301 in ca17eb0
The upper cased "constants" define the expectations in terms of roles: _COORD_DIMS = {
"node_coordinates": "node_dimension",
"edge_coordinates": "edge_dimension",
"face_coordinates": "face_dimension",
}
...
_CONNECTIVITY_DIMS = {
"face_node_connectivity": ("face_dimension", None),
"edge_node_connectivity": ("edge_dimension", 2),
"face_edge_connectivity": ("face_dimension", None),
"face_face_connectivity": ("face_dimension", None),
"edge_face_connectivity": ("edge_dimension", 2),
"boundary_node_connectivity": ("boundary_edge_dimension", 2),
} In terms of types:
(You can argue that mixing names and sizes is a bit (too) pragmatic and that it should be encoded in a namedtuple or dataclass more explicitly.) |
Ok, this would then be something like this: from xugrid.ugrid.conventions import _get_connectivity, _get_topology, _get_coordinates, _get_dimensions
file_nc = dfmt.data.fm_curvedbend_map(return_filepath=True)
ds = xr.open_dataset(file_nc)
# ds = xu.data.adh_san_diego(xarray=True) # 1D topology
topologies = _get_topology(ds)
coordinates = _get_coordinates(ds, topologies=topologies)
connectivities = _get_connectivity(ds, topologies=topologies)
dims = _get_dimensions(ds, topologies=topologies, connectivity=connectivities, coordinates=coordinates)
print(dims) Gives:
We still need to match the variable to the topology name, but that seems easy from here on. It does seem that adding xugrid as a dependency for this is useful (that would simplify the code a lot), or cherry pick the code a bit. |
Ah okay, with your example I get what you're after. I considered something like that in the past, I think you can get all of this in a much nicer form by using the UgridRolesAccessor: https://deltares.github.io/xugrid/api/xugrid.UgridRolesAccessor.html#xugrid.UgridRolesAccessor ds = xr.open_dataset(file_nc)
dim = ds.ugrid_roles["mesh2d"]["node_dimension"] To clarify, this is an xarray accessor that'll work on Datasets. All of this logic is fully contained in the Alternatively, we could split the conventions handling into a separate package, but given the number of packages we're already supporting it's just a lot more convenient to keep things together. |
I knew there should be an easier way to get to this. Thanks for sharing this. This is a simple application of such a topology matching method: import xarray as xr
import dfm_tools as dfmt
file_nc = dfmt.data.fm_curvedbend_map(return_filepath=True)
ds = xr.open_dataset(file_nc)
def get_topodim(ds, varname):
ugrid_roles = ds.ugrid_roles["mesh2d"]
vardims = ds[varname].dims
if ugrid_roles["node_dimension"] in vardims:
topodim = "node"
elif ugrid_roles["face_dimension"] in vardims:
topodim = "face"
elif ugrid_roles["edge_dimension"] in vardims:
topodim = "edge"
else:
topodim = None
return topodim
for varname in ["mesh2d_sa1", "mesh2d_tureps1", "mesh2d_node_z"]:
topodim = get_topodim(ds, varname=varname)
print(f"{varname}: {topodim}") Prints:
You could of course also change this function such that it adds the |
in D-EcoImpact, the
location
attribute is required to map all variables to mesh/node/face locations. This attribute is also described in the conventions (search forMesh2_depth:location = "node"
). However, I cannot judge whether this attribute is required.I expect xugrid just checks which dimensions are present in the variable. Since there can be only one topology dimension per variable and xugrid is aware which these are, it should be quite simple to map these.
There are two possible solutions:
@hrajagers and @Huite could you also state whether this location attribute is required or not?
The text was updated successfully, but these errors were encountered: