[Feature] New Edge Attribute: AttributeFromNode #95
Conversation
icedoom888
changed the title
icedoom888
requested review from
JPXKQX and
OpheliaMiralles
and removed request for
OpheliaMiralles
There was a problem hiding this comment.
Hi Alberto! Thanks for the PR! I have commented on some suggestions for the structure of the new classes. Let me know if you want to comment on any of them. I would also like to add at least some minimal testing and some comments to docs/graphs/edge_attributes.rst.
|
Hey @JPXKQX! Thanks for reviewing this! How do you want to proceed regarding tests and comments of here: docs/graphs/edge_attributes.rst ? |
There was a problem hiding this comment.
For the docstring, I would expect something similar to the EdgeDirection or EdgeLength, with a short explanation of the 2 methods (fromSource, fromTarget) and an example like the following
nodes:
data:
node_builder: ...
attributes:
cutout_mask:
__target__ : anemoi.graphs.nodes.attributes.CutOutMask
edges:
- source_name: ...
target_name: ...
edge_builders: ...
attributes:
comes_from_lam:
_target_: anemoi.graphs.edges.attributes.AttributeFromSourceNode
node_attr_name: cutout_mask
|
|
||
| edge_index = graph[(source_name, "to", target_name)].edge_index | ||
| assert hasattr(graph[node_name], self.node_attr_name) | ||
| val = getattr(graph[node_name], self.node_attr_name).numpy()[edge_index[self.idx]] |
There was a problem hiding this comment.
I would say that direct attribute selection is preferred when possible.
| val = getattr(graph[node_name], self.node_attr_name).numpy()[edge_index[self.idx]] | |
| return graph[node_name][self.node_attr_name].numpy()[edge_index[self.idx]] |
In this case, if we want to test whether the attribute is in the graph, it may be better to use try/catch, with an appropriate error message like.
"{super().__class__.__name__} failed because the attribute {} is not defined for the {} nodes.
Co-authored-by: Mario Santa Cruz <48736305+JPXKQX@users.noreply.github.com>
Co-authored-by: Mario Santa Cruz <48736305+JPXKQX@users.noreply.github.com>
Co-authored-by: Mario Santa Cruz <48736305+JPXKQX@users.noreply.github.com>
Co-authored-by: Mario Santa Cruz <48736305+JPXKQX@users.noreply.github.com>
| @@ -44,3 +44,77 @@ latitude and longitude coordinates of the source and target nodes. | |||
| attributes: | |||
| edge_length: | |||
| _target_: anemoi.graphs.edges.attributes.EdgeDirection | |||
|
|
|||
| ********************* | |||
There was a problem hiding this comment.
I would group all new documentation under the same header "Attribute from Node" (or "Attribute from Source/Target Node"), so we get the left sidebar with the 3 types of attributes: Edge direction, edge length, attribute from node. Now:
I would drop the 1st and 3rd yaml examples, and also the subheaders. If you think it needs some explanation, you can also add a comment after a "#" in the yaml example.
CHANGELOG.md
Outdated
| @@ -24,6 +24,7 @@ Keep it human-readable, your future self will thank you! | |||
| - feat: Support for multiple edge builders between two sets of nodes (#70) | |||
| - feat: Support for providing lon/lat coordinates from a text file (loaded with numpy loadtxt method) to build the graph `TextNodes` (#93) | |||
| - feat: Build 2D graphs with `Voronoi` in case `SphericalVoronoi` does not work well/is an overkill (LAM). Set `flat=true` in the nodes attributes to compute area weight using Voronoi with a qhull options preventing the empty region creation (#93) | |||
| - feat: Add `AttributeFromSourceNode` and `AttributeFromTargetNode` edge attribute to copy attribute from source or target node. Set `node_attr_name` in the config to specify which attribute to copy from the source | target node (#94) | |||
There was a problem hiding this comment.
This should go in the Unreleased section.
Closes ecmwf/anemoi-core#42
📚 Documentation preview 📚: https://anemoi-graphs--95.org.readthedocs.build/en/95/