-
Notifications
You must be signed in to change notification settings - Fork 1
Convert symbolic LTS using LDDs to a BDD based representation #66
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This pull request converts symbolic LTS representation from LDDs to BDDs and begins implementing signature-based strong symbolic bisimulation. The PR also reorganizes third-party dependencies by moving pest_consume and mcrl2-sys to external GitHub repositories.
Changes:
- Adds BDD-based symbolic LTS representation and conversion from LDD format
- Implements initial signature refinement algorithm (incomplete)
- Moves third-party code (
pest_consume,mcrl2-sys) to separate repositories - Adds new CLI commands to
merc-symtool for symbolic LTS operations - Renames mCRL2 features for consistency
Reviewed changes
Copilot reviewed 58 out of 62 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/sym/src/main.rs | Adds new CLI interface with commands for info, explore, reorder, convert, and reduce operations on symbolic LTS |
| crates/symbolic/src/symbolic_lts_bdd.rs | New module implementing BDD-based symbolic LTS representation with conversion from LDD format |
| crates/symbolic/src/sigref.rs | New module for signature refinement algorithm (incomplete implementation) |
| crates/symbolic/src/ldd_to_bdd.rs | Refactors LDD to BDD conversion to use explicit variable mapping instead of auto-generated variables |
| Cargo.toml | Updates dependencies to use external repositories for pest_consume and mcrl2-sys |
| .gitmodules | Removes submodules for mCRL2, boost, and cpptrace (moved to mcrl2-sys repo) |
| 3rd-party/* | Removes vendored pest_consume and mCRL2-related code |
| tools/mcrl2/* | Removes mcrl2-sys crate (moved to separate repository) |
tools/sym/src/main.rs
Outdated
| /// Sets the input symbolic LTS format. | ||
| #[arg(long)] | ||
| format: Option<LtsFormat>, |
Copilot
AI
Jan 20, 2026
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.
The format argument expects SymFormat but the help text says "Sets the input symbolic LTS format", yet the type should be LtsFormat based on the struct name ReduceArgs and the context. This type mismatch will cause compilation errors or incorrect behavior.
…mand to merc-sym.
Also take cases where there are fixpoint but it is not the first one, such as "<true> nu X ..."
…different levels of the partition and the signature. This was unclear from the pseudocode.
…wice as slow with linking as thin LTO. This is 5 minutes in total on a full rebuild for the GUI tools, and is now 2,5 minutes.
* It seems that rusty file dialog has been rewritten to drop about 200 dependencies, so that is nice.
… the miri feature.
3f5ff5a to
bd1aa0d
Compare
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.
Pull request overview
Copilot reviewed 77 out of 83 changed files in this pull request and generated 8 comments.
| let resulting_ldd = bdd_to_ldd(&mut storage, &manager_ref, &bdd, &bits_dd, 0, 0).unwrap(); | ||
|
|
||
| println!("resulting LDD: {}", LddDisplay::new(&storage, &resulting_ldd)); | ||
| assert_eq!(ldd, resulting_ldd, "Converted LDD does not match original"); | ||
| // assert_eq!(ldd, resulting_ldd, "Converted LDD does not match original"); | ||
| }); |
Copilot
AI
Jan 24, 2026
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.
The core correctness assertion in test_random_ldd_to_bdd is commented out, which means this test no longer verifies the LDD↔BDD roundtrip. This likely hides a real regression in ldd_to_bdd/bdd_to_ldd. Please fix the conversion logic so the roundtrip holds again and re-enable the assertion.
| /// Converts an LDD representing a set of vectors into a BDD representing the | ||
| /// same set by bitblasting the vector elements. | ||
| pub fn ldd_to_bdd_simple( | ||
| storage: &mut Storage, | ||
| manager_ref: &BDDManagerRef, | ||
| ldd: &LddRef<'_>, | ||
| ) -> Result<BDDFunction, MercError> { | ||
| let highest = compute_highest(storage, ldd); | ||
| let bits = compute_bits(&highest); | ||
| let bits_dd = merc_ldd::singleton(storage, &bits); | ||
|
|
||
| ldd_to_bdd(storage, manager_ref, ldd, &bits_dd, 0) | ||
| } | ||
|
|
||
| /// Converts an LDD representing a set of vectors into a BDD representing the | ||
| /// same set by bitblasting the vector elements. | ||
| /// same set by bitblasting the vector elements using the given variables as bits. | ||
| /// | ||
| /// # Details | ||
| /// |
Copilot
AI
Jan 24, 2026
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.
The ldd_to_bdd doc comment is now out of sync with the function signature: it still describes a first_variable parameter and implies consecutive BDD layers, but the API now takes an explicit vars: &[VarNo] mapping. Please update the comment to match the new contract (and remove first_variable references).
Also started with the signature based strong symbolic bisimulation. This is not yet working correctly, and the quotient is also missing.