-
Notifications
You must be signed in to change notification settings - Fork 6
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
Portmatching in TKET2 #47
Conversation
# Conflicts: # pyrs/src/lib.rs
Okay, we're at a good point now. Please ignore the recent salvo of commits, the recent (benign) changes to the As suggested by @aborgna-q, we can go ahead and merge this in without filtering out non-convex matches. I've added The last blocker is a release of |
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.
Awesome!
I mostly have some comments about the redefined OpType
s
.github/workflows/ci.yml
Outdated
@@ -33,7 +33,7 @@ jobs: | |||
- name: Run clippy | |||
run: cargo clippy --all-targets -- -D warnings |
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.
Can you add this too? Seems I missed it when defining the features.
run: cargo clippy --all-targets -- -D warnings | |
run: cargo clippy --all-targets --features=$FEATURES -- -D warnings |
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.
done!
src/portmatching/optype.rs
Outdated
impl From<hugr::ops::LeafOp> for LeafOp { | ||
fn from(op: hugr::ops::LeafOp) -> Self { | ||
match op { | ||
hugr::ops::LeafOp::H => LeafOp::H, | ||
hugr::ops::LeafOp::T => LeafOp::T, | ||
hugr::ops::LeafOp::S => LeafOp::S, | ||
hugr::ops::LeafOp::X => LeafOp::X, | ||
hugr::ops::LeafOp::Y => LeafOp::Y, | ||
hugr::ops::LeafOp::Z => LeafOp::Z, | ||
hugr::ops::LeafOp::Tadj => LeafOp::Tadj, | ||
hugr::ops::LeafOp::Sadj => LeafOp::Sadj, | ||
hugr::ops::LeafOp::CX => LeafOp::CX, | ||
hugr::ops::LeafOp::ZZMax => LeafOp::ZZMax, | ||
hugr::ops::LeafOp::Measure => LeafOp::Measure, | ||
hugr::ops::LeafOp::RzF64 => LeafOp::RzF64, | ||
hugr::ops::LeafOp::Xor => LeafOp::Xor, | ||
_ => panic!("Unsupported LeafOp"), | ||
} | ||
} | ||
} | ||
|
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 reference impl should be enough? (you'll need to remove .clone()
in the single place where it's used.
impl From<hugr::ops::LeafOp> for LeafOp { | |
fn from(op: hugr::ops::LeafOp) -> Self { | |
match op { | |
hugr::ops::LeafOp::H => LeafOp::H, | |
hugr::ops::LeafOp::T => LeafOp::T, | |
hugr::ops::LeafOp::S => LeafOp::S, | |
hugr::ops::LeafOp::X => LeafOp::X, | |
hugr::ops::LeafOp::Y => LeafOp::Y, | |
hugr::ops::LeafOp::Z => LeafOp::Z, | |
hugr::ops::LeafOp::Tadj => LeafOp::Tadj, | |
hugr::ops::LeafOp::Sadj => LeafOp::Sadj, | |
hugr::ops::LeafOp::CX => LeafOp::CX, | |
hugr::ops::LeafOp::ZZMax => LeafOp::ZZMax, | |
hugr::ops::LeafOp::Measure => LeafOp::Measure, | |
hugr::ops::LeafOp::RzF64 => LeafOp::RzF64, | |
hugr::ops::LeafOp::Xor => LeafOp::Xor, | |
_ => panic!("Unsupported LeafOp"), | |
} | |
} | |
} |
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.
I've simplified this a bit. My attempt at using macros to make it even simpler failed, so this will have to be good enough.
src/portmatching/optype.rs
Outdated
impl From<hugr::ops::OpType> for OpType { | ||
fn from(op: hugr::ops::OpType) -> Self { | ||
match op { | ||
hugr::ops::OpType::Input(_) => OpType::Input, | ||
hugr::ops::OpType::Output(_) => OpType::Output, | ||
hugr::ops::OpType::LeafOp(op) => OpType::LeafOp(op.into()), | ||
hugr::ops::OpType::LoadConstant(_) => OpType::LoadConstant, | ||
_ => panic!("Unsupported OpType"), | ||
} | ||
} | ||
} | ||
|
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.
impl From<hugr::ops::OpType> for OpType { | |
fn from(op: hugr::ops::OpType) -> Self { | |
match op { | |
hugr::ops::OpType::Input(_) => OpType::Input, | |
hugr::ops::OpType::Output(_) => OpType::Output, | |
hugr::ops::OpType::LeafOp(op) => OpType::LeafOp(op.into()), | |
hugr::ops::OpType::LoadConstant(_) => OpType::LoadConstant, | |
_ => panic!("Unsupported OpType"), | |
} | |
} | |
} |
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.
Done
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.
I don't like that we need these.
-
Are these supposed to match specific operation inhabitants, or could we get away with
OpTag
s? -
If not, will we match parametric operations too? Will we require exact matches for the parameters? What happens if we use floats there?
-
Maybe these should be called something akin "op matchers" instead? (Are these supposed to be disjoint, or can multiple things match the same op?)
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.
I agree this is a bad solution. What I need is to be able to compare and hash OpType
s. The enum variants that cannot auto-derive Eq
and Hash
are irrelevant for our current use cases (things like custom types), and so this is meant to support a subset of OpType
.
Maybe a cleaner solution would be to define
struct MatchOp(OpType);
and implement Hash
and Eq
by hand, and panicking for unsupported ops. What do you think?
We should also be able to match parameters (including floating points). Would you say that this falls outside of the remit of Eq
and should be a custom method MatchOp::approx_equals
?
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.
Ah sorry, this doesn't quite work, given that OpType
has multiple layers of enum
s. I will rename the types.
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.
Uhm. It depends on why do you need Eq
and Hash
in portmatching::NodeProperty
, but if you accept float parameters that's a no-go. Can you use PartialEq
?
Alternatively, we could use something like ordered_float::NotNan
in Hugr and require Hash and Eq on custom ops...
src/portmatching/pyo3.rs
Outdated
pub fn py_from_circuit(circ: PyObject) -> PyResult<CircuitPattern> { | ||
let ser_c = SerialCircuit::_from_tket1(circ); | ||
let hugr: Hugr = ser_c.decode().unwrap(); | ||
hugr.validate() |
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 decoder already runs the validation. (I should probably document that...)
/// A [`hugr::Node`] wrapper for Python. | ||
/// | ||
/// Note: this will probably be useful outside of portmatching |
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.
Node
now implements pyclass in hugr HEAD.
I'll update the dependency here later, so this is OK in the meantime.
(And I'll move __repr__
there).
I see that |
Co-authored-by: Agustín Borgna <[email protected]>
Co-authored-by: Agustín Borgna <[email protected]>
Have a look, I think it's now much better. |
src/portmatching/optype.rs
Outdated
fn ind(&self) -> Option<NonZeroU8> { | ||
match self.0 { | ||
LeafOp::H => NonZeroU8::new(1), | ||
LeafOp::T => NonZeroU8::new(2), | ||
LeafOp::S => NonZeroU8::new(3), | ||
LeafOp::X => NonZeroU8::new(4), | ||
LeafOp::Y => NonZeroU8::new(5), | ||
LeafOp::Z => NonZeroU8::new(6), | ||
LeafOp::Tadj => NonZeroU8::new(7), | ||
LeafOp::Sadj => NonZeroU8::new(8), | ||
LeafOp::CX => NonZeroU8::new(9), | ||
LeafOp::ZZMax => NonZeroU8::new(10), | ||
LeafOp::Measure => NonZeroU8::new(11), | ||
LeafOp::RzF64 => NonZeroU8::new(12), | ||
LeafOp::Xor => NonZeroU8::new(13), | ||
_ => None, | ||
} | ||
} |
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.
What about using the unique name()
? (and maybe s/ind/id/)
fn id(&self) -> Option<SmolStr> {
match self.0 {
LeafOp::H
| LeafOp::T
| LeafOp::S
| LeafOp::X
| LeafOp::Y
| LeafOp::Z
| LeafOp::Tadj
| LeafOp::Sadj
| LeafOp::CX
| LeafOp::ZZMax
| LeafOp::Measure
| LeafOp::RzF64
| LeafOp::Xor => Some(self.0.name()),
_ => None,
}
}
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.
Parametrized ops will still require some changes.
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.
Good idea!
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.
Yes, I'll see about floats when we'll need them.
Co-authored-by: Agustín Borgna <[email protected]>
I think we're good! |
Almost done! The only feature that is missing at this point is filtering out
non-convex matches. Will do this as soon as I've merged in Rust code in the appropriate place.
The integration tests with TKET1 (arguably the most interesting) are in
pyrs/test/test_portmatching.py
.I will add a couple more once everything is done.
In future PRs we should probably refine a bit the APIs, definitely in Python (it's currently terrible),
and possibly in the Rust
src/portmatching
too. I've removed the oldsrc/passes/pattern.rs
code.NB: we need to release a new
portgraph
version for this to compile!