Skip to content
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

Move splitting primitives to serialize library #407

Conversation

saulshanabrook
Copy link
Member

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.

@saulshanabrook saulshanabrook requested a review from a team as a code owner August 6, 2024 20:40
@saulshanabrook saulshanabrook requested review from ajpal and removed request for a team August 6, 2024 20:40
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)
Copy link
Member Author

@saulshanabrook saulshanabrook Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this helper function since it wasn't really doing much

Comment on lines +46 to +48
/// Number of times to inline leaves
#[clap(long, default_value = "0")]
serialize_n_inline_leaves: usize,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the ability to inline leaves via the egglog CLI when making visualizations/json

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()
};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved all this processing into one branch, so it would only be serialized once for JSON or for visualization.

src/sort/set.rs Outdated
Comment on lines 193 to 197

fn serialized_name(&self, _value: &Value) -> Symbol {
"set-of".into()
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/main.rs Outdated Show resolved Hide resolved
@saulshanabrook saulshanabrook force-pushed the serialize-move-split-primitives branch from ea99071 to a555b2f Compare August 7, 2024 18:28
@saulshanabrook saulshanabrook merged commit ae75bb7 into egraphs-good:main Sep 11, 2024
3 checks passed
@saulshanabrook saulshanabrook deleted the serialize-move-split-primitives branch September 11, 2024 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants