feat: Add constraint class and branch selector #771
Conversation
lmondada
requested review from
cqc-alec
and removed request for
a team and
cqc-alec
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## portmatching-v04/base #771 +/- ##
========================================================
Coverage ? 69.38%
========================================================
Files ? 69
Lines ? 8454
Branches ? 8198
========================================================
Hits ? 5866
Misses ? 2203
Partials ? 385
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Ok, I got some way in, not all the way, but time to write up a couple of thoughts.
My first thought is - this is seriously tricky stuff, and we need to be ridiculously precise about tiny differences in meaning etc. to keep this understandable. (Even then it won't be easy, but at least possible ;-)). I'm specifically thinking about "predicate" vs "constraint" here, though that is perhaps not the only example.
My second thought - having worked through some of this - I think maybe we need a bit more of a framework for predicates/constraints. At the moment a portmatching::Constraint stores a Predicate, and a list of args - these will be "constraint variables". We evaluate this by binding the variables to values and then calling the predicate (with the binding map).
A "Constraint Class" is then a Predicate, plus fixed values/variables for some of the inputs, leaving some (maybe exactly one??) of the input (variables) unspecified. Different members of the constraint class then have different values (?) for that final unspecified variable. With me / am I about right so far?
So Constraint is already pretty generic, but I wonder about making ConstraintClass generic too. That is, one way would be for a ConstraintClass to be a Predicate, plus a sparse mapping for predicate-argument-index to variable/value, that omits exactly one argument. (The main thing here is that ConstraintClass is no longer an enum, and does not repeat structure of enum Predicate)
You could then still have a method like get_classes that inspects the constraint, and matches on the Predicate inside, before returning a list of ConstraintClasses. But the tough-to-understand docs on the ConstraintClass variants (tough partly because they were written in English!) would disappear, and/or be replaced by the specification inside get_classes (which I think would be more precise and easier by being in code).
Next level up, add metadata to enum Predicate - a function Predicate::list_argument_types(&self) -> List<ArgumentType>, where you have something like enum ArgumentType { Node, PortOffset, Wire, Op, .... }. You could then add some debug_asserts that each constructed instance of ConstraintClass left exactly one parameter unfilled, sort of thing.
Going even further....not sure about this but....currently Predicate itself forces you to specify some inputs statically (as fields in the Predicate instance) and provide others as arguments. One could generalize: make Predicate stateless, just a function from a load of arguments to bool. The Constraint then binds each Predicate input (remember, there's metadata in list_argument_types) to either a constraint-variable or a fixed value (previously a field inside Predicate but not now). I think that would make ConstraintClass even more uniform (as presently ConstraintClass stores both Predicate-fields and Predicate-arguments)...
| //! Constraint classes for [`Predicate`]. | ||
| //! | ||
| //! Constraint classes are labels for constraints that are used to cluster | ||
| //! constraints into groups that are related to one another (in some way), and |
There was a problem hiding this comment.
| //! constraints into groups that are related to one another (in some way), and | |
| //! constraints into groups that are related to one another (in that they should | |
| //! be evaluated together - e.g. this might be more efficient) |
| //! classes, as well as the "expansion factor" of a list of constraints | ||
| //! (a heuristic to determine how "valuable/expensive" constraints are -- | ||
| //! see the inline comments for more details). | ||
| //! 2. Provides a branch selector [`BranchSelector`] to evaluate lists of |
There was a problem hiding this comment.
No, SelectorWithinGroup. Ok, maybe BranchSelector is not so bad.
| /// evaluated and constructed together. | ||
| /// | ||
| /// These dictate the set of available transitions within a pattern matcher: all | ||
| /// outgoing transitions at any one state will share one constraint class. |
There was a problem hiding this comment.
This "All outgoing transitions at any one state will share a constraint class" seems like much more a statement than (most versions of) the previous, so might be the first thing to say. Indeed, maybe we should think of this as less "ConstraintClass" and more "Branch" or "Test" (the latter could go with renaming "BranchSelector" to "Tester" i.e. something which does Tests)
There was a problem hiding this comment.
The phrase is often "test-and-branch" i.e. the test determines which branch to take
| }; | ||
|
|
||
| /// The constraint classes that cluster hugr [`Predicate`]s into groups that can be | ||
| /// evaluated and constructed together. |
There was a problem hiding this comment.
Note there are three possible statements here:
- Predicates can be evaluated together. Note that this allows them to be evaluated separately, so doesn't really say very much.
- Predicates should be evaluated together. A bit stronger than the previous
- Predicates must be evaluated together, i.e. they cannot be evaluated separately. Stronger again.
(Ok, assuming you want to say the same thing about evaluation and construction!)
Which do you mean?
There was a problem hiding this comment.
Also, are we clustering Predicates together or Constraints? I think the latter, i.e. predicates on particular variables??
| /// The class is parametrised by the first wire of the predicate -- the one | ||
| /// that is distinct from all others. The logical relationship between two | ||
| /// constraints of the same class is given by the relationship between the | ||
| /// sets of all but the first predicate argument: e.g. if one's set is a | ||
| /// subset of another, then there is a logical implication from the second |
There was a problem hiding this comment.
| /// The class is parametrised by the first wire of the predicate -- the one | |
| /// that is distinct from all others. The logical relationship between two | |
| /// constraints of the same class is given by the relationship between the | |
| /// sets of all but the first predicate argument: e.g. if one's set is a | |
| /// subset of another, then there is a logical implication from the second | |
| /// The class is parametrised by the first wire given to the predicate, i.e. the "query" | |
| /// that must be distinct from all the others. One such constraint implies another | |
| /// if it has a strictly larger set of `others` (i.e. that the query must be distinct from) |
Note introduction of "query", which could be noted in the doc for Predicate::IsDistinctFrom, and I'm trying to be very rigorous in predicate-vs-constraint.
Also, it'd be good to comment (in Predicate::IsDistinctFrom) why n_others: cmp::Reverse<usize> is what it is - i.e. that it just stores a usize, but the cmp::Reverse is just so that Predicates with greater n_others are ordered before those with fewer. (Ideally it'd be nice to remove such ordering behaviour from the storage representation but then you'd have to implement PartialEq yourself which is a massive chunk of code, so don't do that ;) )
| /// | ||
| /// The class is parametrised by the node and port the wire is connected to. | ||
| /// There can only be one wire at any one port. Hence any two constraints | ||
| /// in the same class must be connected to the same wire or be mutually |
There was a problem hiding this comment.
must be connected to the same wire does that make them equivalent? (I.e. any two constraints in the class must be equivalent or mutually exclusive?)
| /// of a wire is always unique. Hence any two constraints on the source of | ||
| /// the same wire must be for the same node and port or be mutually | ||
| /// exclusive. | ||
| IsWireSourceClass(HugrPortID), |
There was a problem hiding this comment.
So a HugrPortID is actually a HugrNodeID plus a hugr::Port. That's nearly the same contents as an OccupyOutgoingPortClass (which is a HugrNodeID plus a hugr::OutgoingPort). Are they in fact the same contents?
There was a problem hiding this comment.
Indeed I am trying to understand the difference between this and OccupyOutgoingPortClass (given both are classes of Predicate::IsWireSource). Is it about the direction of the port in the class? I note this one talks about "the source of a wire is always unique" so this one is about an outgoing port (i.e. source), whereas OccupyOutgoingPortClass talks about "the node and port the wire is connected to" so might be about an incoming port?? (But the code says otherwise - OccOutPC contains an OutgoingPort whereas this contains an arbitrary port)
If the difference is direction, maybe we could have one storing an OutgoingPort and one storing an IncomingPort? (Or, can combine the two enum-variants by storing a hugr::Port, interpreting it depending on direction)
There was a problem hiding this comment.
Actually, I think my confusion relates to:
Predicate::IsWireSource(OutPort) :: Wire, Node -> Bool
and the difference is that in one of these constraint classes, the OutPort and Node are fixed, whereas in the other....the Wire is fixed? (Or maybe the OutPort and the node of the wire, or one endpoint of the wire, or something like that)
There was a problem hiding this comment.
See overall review comment!
| Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, serde::Serialize, serde::Deserialize, | ||
| )] | ||
| pub enum ConstraintClass { | ||
| /// The class of all [`Predicate::IsOpEqual`] predicates. |
There was a problem hiding this comment.
Strictly speaking I think one of these is the class of all [Predicate::IsOpEqual] predicates on the same variable?
| IsLinearWireSinkClass(HugrPortID), | ||
| } | ||
|
|
||
| impl pm::ConstraintClass<super::Constraint> for ConstraintClass { |
There was a problem hiding this comment.
At first I thought, surely this should be:
impl super::Constraint {
fn get_classes(&self) -> Vec<ConstraintClass<Constraint>> {
But thinking a bit more - maybe this is the right place, let's keep "the next level up" separate from the stuff it's built on. How about renaming from ConstraintClass::get_classes to `ConstraintClass::all_containing" ?
| } | ||
| }) | ||
| }) | ||
| .count() as u64 |
There was a problem hiding this comment.
nit: as _ or as portmatching::constraint_class::ExpansionFactor
This is the third PR that aims to update tket2 to the new APIs of portmatching. It does not merge into main yet, but into a dedicated branch that will eventually be merged atomically into
mainonce everything is ready to go.Overview
#755 introduced
Predicates andConstraints for pattern matching on Hugrs. This PR defines "constraint classes" and "branch selectors" for these predicates and constraints.I've spent a lot of time documenting this code, I hope it's an easy read! Let me know if you have any questions.