Move splitting primitives to serialize library #407
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
saulshanabrook
commented
Aug 6, 2024
Comment on lines
-1558
to
-1570
| pub fn serialize_for_graphviz( | ||
| &self, | ||
| split_primitive_outputs: bool, | ||
| max_functions: usize, | ||
| max_calls_per_function: usize, | ||
| ) -> egraph_serialize::EGraph { | ||
| let config = SerializeConfig { | ||
| split_primitive_outputs, | ||
| max_functions: Some(max_functions), | ||
| max_calls_per_function: Some(max_calls_per_function), | ||
| ..Default::default() | ||
| }; | ||
| self.serialize(config) |
There was a problem hiding this comment.
I removed this helper function since it wasn't really doing much
saulshanabrook
commented
Aug 6, 2024
Comment on lines
+46
to
+48
| /// Number of times to inline leaves | ||
| #[clap(long, default_value = "0")] | ||
| serialize_n_inline_leaves: usize, |
There was a problem hiding this comment.
I added the ability to inline leaves via the egglog CLI when making visualizations/json
saulshanabrook
commented
Aug 6, 2024
src/main.rs
Outdated
Comment on lines
212
to
233
| // if we are splitting primitive outputs, add `-split` to the end of the file name | ||
| let serialize_filename = if args.serialize_split_primitive_outputs { | ||
| input.with_file_name(format!( | ||
| "{}-split", | ||
| input.file_stem().unwrap().to_str().unwrap() | ||
| )) | ||
| } else { | ||
| input.clone() | ||
| }; | ||
| if args.to_json { | ||
| let json_path = serialize_filename.with_extension("json"); | ||
| let config = SerializeConfig { | ||
| split_primitive_outputs: args.serialize_split_primitive_outputs, | ||
| ..SerializeConfig::default() | ||
| }; | ||
| let serialized = egraph.serialize(config); | ||
| serialized.to_json_file(json_path).unwrap(); | ||
| } | ||
|
|
||
| if args.to_dot || args.to_svg { | ||
| let serialized = egraph.serialize_for_graphviz( | ||
| args.serialize_split_primitive_outputs, | ||
| args.max_functions, | ||
| args.max_calls_per_function, | ||
| ); | ||
| if args.to_json || args.to_dot || args.to_svg { | ||
| let mut serialized = egraph.serialize(SerializeConfig::default()); | ||
| if args.serialize_split_primitive_outputs { | ||
| serialized.split_e_classes(|id, _| egraph.from_node_id(id).is_primitive()) | ||
| } | ||
| for _ in 0..args.serialize_n_inline_leaves { | ||
| serialized.inline_leaves(); | ||
| } | ||
|
|
||
| // if we are splitting primitive outputs, add `-split` to the end of the file name | ||
| let serialize_filename = if args.serialize_split_primitive_outputs { | ||
| input.with_file_name(format!( | ||
| "{}-split", | ||
| input.file_stem().unwrap().to_str().unwrap() | ||
| )) | ||
| } else { | ||
| input.clone() | ||
| }; |
There was a problem hiding this comment.
I moved all this processing into one branch, so it would only be serialized once for JSON or for visualization.
saulshanabrook
commented
Aug 6, 2024
src/sort/set.rs
Outdated
Comment on lines
193
to
197
|
|
||
| fn serialized_name(&self, _value: &Value) -> Symbol { | ||
| "set-of".into() | ||
| } | ||
|
|
There was a problem hiding this comment.
This is an unrelated change... It means that when set constructors are serialized they will use this as their name instead of the name of the sort.
saulshanabrook
commented
Aug 7, 2024
saulshanabrook
force-pushed
the
serialize-move-split-primitives
branch
from
ea99071
to
a555b2f
Compare
yihozhang
approved these changes
Aug 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR moves the ability to split primitive nodes into the serialize library from this library and also extends it to support splitting arbitrary nodes. See egraphs-good/egraph-serialize#14 for the upstream PR that corresponds to this one, which includes an example using this new behavior.
By default, this should result in the same behavior as before, but just with the ability to split more e-classes besides just primitive nodes if desired.
In order to move the splitting logic upstream, I exposed a way to check if a serialized node ID is a primitive. In a way, this extends #396 by also now supporting public deserialization/serialization for serialized node IDs as well as class IDs. This should also be useful if you do extraction externally and then want to convert back to egglog values.