-
Notifications
You must be signed in to change notification settings - Fork 11
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
[feature] support post-processors #71
Conversation
…nemoi-graphs into feature/remove-unconnected-nodes
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.
There is a bug in GraphDescriptor
if one drops unconnected nodes from the graph. In its function get_node_summary()
please change
attributes.remove("x")
to
exclude = ["x","indices_connected_nodes"]
attributes = [a for a in attributes if a not in exclude]
Edit: Actually the above is not a proper fix, since "indices_connected_nodes"
is a name specified in the config. So it should be replaced with the corresponding config entry or another [more robust?] solution needs to be found.
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.
Similarly to the bug in GraphDescriptor
there is one in plotting.prepare.py. In the function get_node_attribute_dims
please change
attr_dims = {}
for nodes in graph.node_stores:
for attr in nodes.node_attrs():
if attr == "x" or not isinstance(nodes[attr], torch.Tensor):
continue
to
attr_dims = {}
exclude = ["x","indices_connected_nodes"]
for nodes in graph.node_stores:
for attr in nodes.node_attrs():
if attr in exclude or not isinstance(nodes[attr], torch.Tensor):
continue
Edit: Actually the above is not a proper fix, since "indices_connected_nodes" is a name specified in the config. So it should be replaced with the corresponding config entry or another [more robust?] solution needs to be found.
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.
Could be nicer if in BaseMaskingProcessor
the mask is stored as self.mask
rather than passed along as an argument between each of the routines. The compute_mask
function of subclasses could then set that mask attribute.
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.
Looks good to me now. Thanks Mario!
This PR allows the user to create their own post-processors for the graph. In particular, we provide an implementation of
RemoveUnconnectedNodes
which may be useful for LAM, where we want to remove the nodes far from the area of interest.The post-processors are included in the recipe as follows:
Here, we have 2 optional arguments:
ignore
: It allows the user to ignore some nodes based on an existing attribute. For example, do not drop nodes from inside the limited area, even if they are not connected.save_mask_indices_to_attr
: It allows the user to store the indices to mask the nodes from the previous graph to the processed graph as an attribute. This may be needed for training/inference using only the points of interest.