-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat(graphs): New Edge Attribute: AttributeFromNode #62
base: develop
Are you sure you want to change the base?
feat(graphs): New Edge Attribute: AttributeFromNode #62
Conversation
Co-authored-by: Mario Santa Cruz <[email protected]>
Co-authored-by: Mario Santa Cruz <[email protected]>
Co-authored-by: Mario Santa Cruz <[email protected]>
Co-authored-by: Mario Santa Cruz <[email protected]>
…edge_attr_from_node_attr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Alberto, I posted a few questions below. First of all, thank you again for your new contribution. It is good to see the documentation updated with the PR, could you also add some minor tests?
@@ -14,6 +14,9 @@ Keep it human-readable, your future self will thank you! | |||
|
|||
### Added | |||
|
|||
- 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) | |||
|
|||
# Changed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this added?
target_name: hidden | ||
edge_builders: ... | ||
attributes: | ||
cutout: # Assigned name to the edge attribute, can be different than node_attr_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use something more meaningful as an example? comes_from_cutout, from_cutout, ...
target_name: data | ||
edge_builders: ... | ||
attributes: | ||
cutout: # Assigned name to the edge attribute, can be different than node_attr_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as before
@@ -35,9 +36,9 @@ def post_process(self, values: np.ndarray) -> torch.Tensor: | |||
if values.ndim == 1: | |||
values = values[:, np.newaxis] | |||
|
|||
normed_values = self.normalise(values) | |||
norm_values = self.normalise(values) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
norm_values
can refer to the norm of the values. If you think normed_values is confusing, I suggest we use normalised_values
or we just overwrite values
.
super().__init__(norm=None, dtype="bool") | ||
|
||
|
||
class AttributeFromNode(BooleanBaseEdgeAttribute, ABC): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add the prefix Base
? to make it clear that this class should not be instantiated.
@@ -263,7 +263,7 @@ class CutOutMask(BooleanBaseNodeAttribute): | |||
|
|||
def get_raw_values(self, nodes: NodeStorage, **kwargs) -> np.ndarray: | |||
assert isinstance(nodes["_dataset"], dict), "The 'dataset' attribute must be a dictionary." | |||
assert "cutout" in nodes["_dataset"], "The 'dataset' attribute must contain a 'cutout' key." | |||
assert "cutout" in nodes["_dataset"] or "multivariablecutout" in nodes["_dataset"], "The 'dataset' attribute must contain a 'cutout' key." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change is not in line with this PR.
Closes #42