graph edges `level` attribute is inconsistent across different `direction` types

[observingClouds]

The level attribute within a mesh level, e.g. (direction == same), is called "level", opposed to when the edges connect different mesh levels. In those cases it is called "levels".

E.g.

weather-model-graphs/src/weather_model_graphs/create/mesh/kinds/hierarchical.py

Line 108 in 3d9d1ef

G_down.edges[u, v]["levels"] = f"{from_level}>{to_level}"

vs.

weather-model-graphs/src/weather_model_graphs/create/mesh/kinds/hierarchical.py

Line 68 in 3d9d1ef

G.edges[u, v]["level"] = i

This causes issues, e.g. when using https://github.com/pyg-team/pytorch_geometric/blob/afffee9f089ac0fe1eb8c1600c6e0b2e07ca6f80/torch_geometric/utils/convert.py#L260-L262

[joeloskarsson]

While it is of course a problem that this causes downstream issues, I am not sure if it is a good idea for these both features to be called the same thing. One is a string and one is a an integer, and a situation where the level attribute is sometimes a string and sometimes an integer seems undesirable. The notion of level for the inter-level edges is also ambiguous, as they start from one level and end in another. Perhaps a better setup would be to have start_level and end_level (integer) attributes for these edges?

With that said, it is of course a problem that this causes issues in the from_networkx function. But is that not a problem with that function rather than the design of the graph attributes then? That check seems quite restrictive, and I guess now even goes against our own design 😛

[leifdenby]

The level attribute within a mesh level, e.g. (direction == same), is called "level", opposed to when the edges connect different mesh levels. In those cases it is called "levels".

Yes, this was by design, but maybe it is a bad idea. I wanted to make it clear that these attributes specify different things, as in the case of levels it defines which levels are connected and in the case of level it defines the level within-which the edges connections exist. Do you have a different way you would like to do this @observingClouds?

[observingClouds]

My main concern here was mostly related to the upstream compatibility.

With the explanations given here, I still think the attribute names could be more explicit about their nature (e.g. connection_level and connection). Or would it be possible to merge these two to connection, which is f"{from_level}>{to_level}" in all cases, even in case of connections within a level, resulting in e.g. 1>1 or 1<>1?