Add ability to split e-classes for easier visualization #14
Conversation
src/algorithms.rs
Outdated
| for Class { id, nodes } in self.classes().clone().values() { | ||
| let mut other_nodes = Vec::new(); | ||
| let mut unique_node = None; | ||
| for node_id in nodes { |
There was a problem hiding this comment.
Nits: the logic can be probably simplified with parition
There was a problem hiding this comment.
Thank you, yep that was much clearer.
src/algorithms.rs
Outdated
| /// | ||
| /// This can be used for example to make multiple e-classes for all nodes equivalent to i64(0), to make it easier | ||
| /// to visualize this. | ||
| pub fn split_e_classes(&mut self, should_split: impl Fn(&NodeId, &Node) -> bool) { |
There was a problem hiding this comment.
I found myself reading the description and the implementation several times before understanding what this does. The "unique node" is essentially a representative that every duplicated e-class would contain, and that's why it needs to be unique, is this right? I think the documentation might be clearer if it is narrated from an application point of view (e.g., what it tries to achieve at a high level) or describes some end-to-end use scenarios.
There was a problem hiding this comment.
nits: eclasses or classes seems to be more consistent with the rest of the library than e_classes
There was a problem hiding this comment.
Thank you! I updated the comment to try to explain it a bit more. Happy for more feedback about parts that are confusing.
| } | ||
| // split out other nodes if there are multiple of them. | ||
| // Leave one node in this e-class and make new e-classes for remaining nodes | ||
| for other_node_id in other_nodes.into_iter().skip(1) { |
There was a problem hiding this comment.
For other_nodes[0], which is skipped, while you don't need to create a new e-class for it, don't you need to merge it with a clone of unique_node?
There was a problem hiding this comment.
No, I don't believe so because we leave the original unique node there with it.
| // split out other nodes if there are multiple of them. | ||
| // Leave one node in this e-class and make new e-classes for remaining nodes | ||
| for other_node_id in other_nodes.into_iter().skip(1) { | ||
| changed = true; |
There was a problem hiding this comment.
Why is changed set to true here (in addition to below where parent E-nodes are updated).
There was a problem hiding this comment.
I updated the logic to only trigger changed if there are any parents and we make a new node. Changed so only be true if the e-graph was mutated.
|
@mwillsey Could you take a look at this? egraphs-good/egglog#407 is blocked on it |
This PR adds the ability to "split" e-classes to assist with visualization.
This feature was previously implemented as part of the egglog export process, to allow splitting primitive nodes into their own classes.
For example, this is an unsplit e-class:
And after splitting primitive nodes:
By moving this ability to the serialize library and extending it to support arbitrary nodes, we can use it to split other nodes besides primitives. For example, we can choose to split
Numnodes or other nodes that are contr. I implemented this to help when debugging e-graphs through visualization, by reducing the cyclic nature of the graphs.For example, here is an e-graph with just primitives split and then inlined (old behavior):
And then with this PR, it is a bit easier to follow the top down flow of the nodes (ignoring the isolated nodes in the top right):