feat!: Rewriter API + ECCRewriter #93
Conversation
|
I've broken the PR into commits that should help reviewing. Happy to break this into multiple PRs if you prefer. |
# Conflicts: # src/lib.rs
src/passes/taso.rs
Outdated
| /// | ||
| /// An ECC always has a representative circuit, so it will be of size at | ||
| /// least 1. | ||
| #[allow(clippy::len_without_is_empty)] |
There was a problem hiding this comment.
I guess we could avoid having to suppress this warning by having an is_empty() that returns false. Or we could call this method something else e.g. n_others.
There was a problem hiding this comment.
Renaming is a good option. I've chosen n_circuits.
src/portmatching/matcher.rs
Outdated
| circ: &'a C, | ||
| ) -> Result<Self, InvalidCircuitMatch> { | ||
| let mut checker = ConvexChecker::new(circ); | ||
| Self::try_from_root_match_with_checker(root, pattern, circ, &mut checker) | ||
| Self::try_from_root_match_with_checker(root, pattern_id, pattern, circ, &mut checker) |
There was a problem hiding this comment.
Feels odd to be passing around both the pattern ID and the pattern; can the pattern be retrieved from the ID?
There was a problem hiding this comment.
Mmmm, I agree that this is ugly. The problem is that the pattern ID is only unique to a CircuitMatcher object, so the alternative is to carry around a reference to a matcher.
I might break this into separate calls.
src/rewrite/ecc_rewriter.rs
Outdated
| /// Rewrites, stored as a map from PatternID to TargetIDs. | ||
| rewrite_rules: Vec<Vec<TargetID>>, |
There was a problem hiding this comment.
This doesn't look like a map from PatternID to TargetIDs. Presumably the pattern ID is used as the index into the outer vector. Suggest rewording docstring.
There was a problem hiding this comment.
Yes that's true, I've made this clearer.
src/rewrite/ecc_rewriter.rs
Outdated
| //! generates rewrites to replace them with other circuits within the same | ||
| //! equivalence class. | ||
| //! | ||
| //! Equivalence classes are generated using Quartz. |
There was a problem hiding this comment.
Should include a reference here.
| impl ECCRewriter { | ||
| /// Create a new rewriter from equivalent circuit classes in JSON file. | ||
| /// | ||
| /// This uses the Quartz JSON file format to store equivalent circuit classes. |
src/rewrite/ecc_rewriter.rs
Outdated
| let all_hugrs = rep_sets.iter().flat_map(|rs| rs.circuits()); | ||
| let all_circs = all_hugrs | ||
| .map(|hugr| SiblingGraph::<DfgID>::new(hugr, hugr.root())) | ||
| // TODO: collect to vec because of lifetime issues that should not exist |
There was a problem hiding this comment.
What's the TODO? It seems we are collecting to vec ... is the TODO to not do this?
There was a problem hiding this comment.
Indeed, I meant the opposite. This is an issue that I'm unable to solve -- every time I try to refactor the relevant lifetime bounds it fails, so will leave this to someone more skilled than me.
I don't think this will be a bottleneck in terms of performance so I can also remove the TODO if you prefer, but I found it is reasonable to highlight this problem here. For the time being I've rephrased the TODO and made my point clearer.
Co-authored-by: Alec Edgington <[email protected]>
|
Removing the duplicate The main code is definitely cleaner now, at the expense of slightly messier Python bindings. I think that's a fair tradeoff, especially given that the Python layer really is just a prototype at the moment. EDIT: I also took the opportunity to rename |
src/portmatching/pyo3.rs
Outdated
| /// The incoming port boundary. | ||
| pub inputs: Vec<Vec<(Node, Port)>>, |
There was a problem hiding this comment.
Probably needs some more explanation in the doc as it's not obvious how a Vec<Vec<(Node, Port)>> corresponds to an incoming port boundary (nor why it's a different type from the outgoing boundary).
src/portmatching/matcher.rs
Outdated
| circ: &'a C, | ||
| ) -> Result<Self, InvalidCircuitMatch> { | ||
| matcher: &CircuitMatcher, |
There was a problem hiding this comment.
Should we rename this to PatternMatcher?
There was a problem hiding this comment.
Yes, I guess everything in tket2 relates to circuits, so Circuit prefixes are generally redundant.
Co-authored-by: Alec Edgington <[email protected]>
- refactor!: Standardise ECC naming - feat: Add Subcircuit and move CircuitRewrite - refactor: CircuitMatch stores PatternID - feat: Add ECCRewriter BREAKING CHANGE: `CircuitMatch` is now `PatternMatch`, `CircuitMatcher` is now `PatternMatcher`. BREAKING CHANGE: RepCircSet is now EqCircClass. BREAKING CHANGE: rep_sets_from_path is now load_eccs_json_file. BREAKING CHANGE: CircuitMatch.pattern is now a PatternID instead of a reference and no longer has a 'p pattern lifetime. BREAKING CHANGE: CircuitRewrite was moved to ::rewrite.
BREAKING CHANGE: RepCircSet is now EqCircClass.
BREAKING CHANGE: rep_sets_from_path is now load_eccs_json_file.
BREAKING CHANGE: CircuitMatch.pattern is now a PatternID instead of a reference and no longer
has a 'p pattern lifetime.
BREAKING CHANGE: CircuitRewrite was moved to ::rewrite.