Portmatching in TKET2 #47

lmondada · 2023-07-27T15:05:23Z

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 old src/passes/pattern.rs code.

NB: we need to release a new portgraph version for this to compile!

# Conflicts: # pyrs/src/lib.rs

lmondada · 2023-07-31T11:41:38Z

Okay, we're at a good point now. Please ignore the recent salvo of commits, the recent (benign) changes to the pyrs folder caught me off guard.

As suggested by @aborgna-q, we can go ahead and merge this in without filtering out non-convex matches. I've added TODOs in the test for the time being.

The last blocker is a release of portmatching, which I'll do asap.

aborgna-q

Awesome!

I mostly have some comments about the redefined OpTypes

aborgna-q · 2023-07-31T11:46:45Z

.github/workflows/ci.yml

@@ -33,7 +33,7 @@ jobs:
      - name: Run clippy
        run: cargo clippy --all-targets -- -D warnings


Can you add this too? Seems I missed it when defining the features.

Suggested change

run: cargo clippy --all-targets -- -D warnings

run: cargo clippy --all-targets --features=$FEATURES -- -D warnings

src/portmatching.rs

aborgna-q · 2023-07-31T13:22:58Z

src/portmatching/optype.rs

+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"),
+        }
+    }
+}
+


The reference impl should be enough? (you'll need to remove .clone() in the single place where it's used.

Suggested change

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"),

}

}

}

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.

aborgna-q · 2023-07-31T13:23:30Z

src/portmatching/optype.rs

+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"),
+        }
+    }
+}
+


Suggested change

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"),

}

}

}

aborgna-q · 2023-07-31T13:27:52Z

src/portmatching/optype.rs

I don't like that we need these.

Are these supposed to match specific operation inhabitants, or could we get away with OpTags?

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?)

I agree this is a bad solution. What I need is to be able to compare and hash OpTypes. 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?

Ah sorry, this doesn't quite work, given that OpType has multiple layers of enums. I will rename the types.

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...

aborgna-q · 2023-07-31T13:29:39Z

src/portmatching/pyo3.rs

+    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()


The decoder already runs the validation. (I should probably document that...)

aborgna-q · 2023-07-31T13:33:16Z

src/portmatching/pyo3.rs

+/// A [`hugr::Node`] wrapper for Python.
+///
+/// Note: this will probably be useful outside of portmatching


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).

src/portmatching/matcher.rs

aborgna-q · 2023-07-31T13:49:57Z

I see that maturin is failing in CI due to environment configs.
I'd remove the CI job for now; I'll add it using the devenv config later: https://github.com/CQCL-DEV/tket2/issues/50

Co-authored-by: Agustín Borgna <121866228+aborgna-q@users.noreply.github.com>

lmondada · 2023-07-31T15:27:28Z

Have a look, I think it's now much better.

src/portmatching/pyo3.rs

aborgna-q · 2023-07-31T15:44:20Z

src/portmatching/optype.rs

+    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,
+        }
+    }


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, } }

Parametrized ops will still require some changes.

Yes, I'll see about floats when we'll need them.

Co-authored-by: Agustín Borgna <121866228+aborgna-q@users.noreply.github.com>

lmondada · 2023-07-31T16:03:48Z

I think we're good!

lmondada added 12 commits July 25, 2023 11:41

Add portmatching feature

fb54212

Add pattern and matcher

a5d411c

Python bindings

0f1c6a4

Merge branch 'main' into feature/portmatching

ae942d9

# Conflicts: # pyrs/src/lib.rs

Update portgraph

e2a3d41

Add python tests + CI

8493958

Merge branch 'main' into feature/portmatching

a6ec189

Clean up merge

e6c886d

Clean up merge (2)

b7e5a80

Clean up merge (2)

fbb2f6e

Fix test

8105250

Fix CI

00e79f2

Update portmatching

21bc785

lmondada marked this pull request as ready for review July 31, 2023 12:12

Fix CI

2165abc

aborgna-q approved these changes Jul 31, 2023

View reviewed changes

lmondada and others added 4 commits July 31, 2023 15:50

Do not use maturin develop in CI

a2965a6

Update src/portmatching.rs

ed51290

Co-authored-by: Agustín Borgna <121866228+aborgna-q@users.noreply.github.com>

Update src/portmatching/matcher.rs

ac065e1

Co-authored-by: Agustín Borgna <121866228+aborgna-q@users.noreply.github.com>

Address comments

357a0a8

aborgna-q self-requested a review July 31, 2023 15:44

aborgna-q reviewed Jul 31, 2023

View reviewed changes

lmondada and others added 5 commits July 31, 2023 17:49

Update src/portmatching/pyo3.rs

59eec4f

Co-authored-by: Agustín Borgna <121866228+aborgna-q@users.noreply.github.com>

Change ind -> name

4efc5d7

Fix hashing

cc90252

Fix hashing!

9ab7dfa

Update readme

137c690

lmondada merged commit 1454f8e into main Jul 31, 2023
5 checks passed

lmondada deleted the feature/portmatching branch July 31, 2023 17:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Portmatching in TKET2 #47

Portmatching in TKET2 #47

lmondada commented Jul 27, 2023 •

edited

Loading

lmondada commented Jul 31, 2023

aborgna-q left a comment

aborgna-q Jul 31, 2023

lmondada Jul 31, 2023

aborgna-q Jul 31, 2023

lmondada Jul 31, 2023

aborgna-q Jul 31, 2023

lmondada Jul 31, 2023

aborgna-q Jul 31, 2023

lmondada Jul 31, 2023

lmondada Jul 31, 2023

aborgna-q Jul 31, 2023

aborgna-q Jul 31, 2023

aborgna-q Jul 31, 2023

aborgna-q commented Jul 31, 2023 •

edited

Loading

lmondada commented Jul 31, 2023

aborgna-q Jul 31, 2023

aborgna-q Jul 31, 2023

lmondada Jul 31, 2023

lmondada Jul 31, 2023

lmondada commented Jul 31, 2023

		@@ -33,7 +33,7 @@ jobs:
		- name: Run clippy
		run: cargo clippy --all-targets -- -D warnings

	run: cargo clippy --all-targets -- -D warnings
	run: cargo clippy --all-targets --features=$FEATURES -- -D warnings

Portmatching in TKET2 #47

Portmatching in TKET2 #47

Conversation

lmondada commented Jul 27, 2023 • edited Loading

lmondada commented Jul 31, 2023

aborgna-q left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aborgna-q commented Jul 31, 2023 • edited Loading

lmondada commented Jul 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmondada commented Jul 31, 2023

lmondada commented Jul 27, 2023 •

edited

Loading

aborgna-q commented Jul 31, 2023 •

edited

Loading