test: TASO termination in simple cases #155
Conversation
|
So the answer to #152 is that it does terminate? |
Yes. What happens though in practice is that for a cost function such as CX gate count and sufficiently many rewrite rules, there are infinitely many circuits: you can always add new single-qubit gates. You could try to bound the total size of the circuit considered to also terminate in that case, but I think in practice a finite capacity priority queue and some smarter termination criterion is probably better. |
There was a problem hiding this comment.
Is there any way to put a comment in this file saying what it is? I think I remember that this is an annoying deficiency of json, so I don't know if you can add a _comment field or something? It'd be good to know whether this is golden test output, test inputs, etc...
There was a problem hiding this comment.
Hmmm, as you say, no easy way to add comments. And as this particular JSON is an array, no easy way to add spare fields either.
I've at least added a doc in the test file so that if people search for the file name they will quickly see where and why it is used. The name is also fairly descriptive (provided you are familiar with the terms Nam and ECC:P).
Is that good enough for you?
tests/taso_termination.rs
Outdated
| #[fixture] | ||
| fn simple_circ() -> Hugr { | ||
| let json = r#" | ||
| {"bits": [], "commands": [{"args": [["q", [0]], ["q", [2]]], "op": {"type": "CX"}}, {"args": [["q", [0]]], "op": {"params": ["0.1"], "type": "Rz"}}, {"args": [["q", [0]], ["q", [1]]], "op": {"type": "CX"}}, {"args": [["q", [1]]], "op": {"type": "H"}}, {"args": [["q", [1]]], "op": {"params": ["0.2"], "type": "Rz"}}, {"args": [["q", [1]]], "op": {"type": "H"}}, {"args": [["q", [0]], ["q", [1]]], "op": {"type": "CX"}}, {"args": [["q", [0]], ["q", [2]]], "op": {"type": "CX"}}, {"args": [["q", [0]]], "op": {"params": ["-0.1"], "type": "Rz"}}], "created_qubits": [], "discarded_qubits": [], "implicit_permutation": [[["q", [0]], ["q", [0]]], [["q", [1]], ["q", [1]]], [["q", [2]], ["q", [2]]]], "phase": "0.0", "qubits": [["q", [0]], ["q", [1]], ["q", [2]]]} |
There was a problem hiding this comment.
Would be good to spread this over a few lines and indent/layout it a bit. (That even gives room for comments, although I'm not sure whether any are necessary)
I note in particular the implicit_permutation - that's with respect to some kind of underlying notion of the natural ordering, presumably (?), so I'm wondering where that comes from - e.g. whether this is just a property of whether the optimizer has ever seen a different permutation of the same thing before?
There was a problem hiding this comment.
This is just a lazy way for me to define a circuit. It's extremely verbose and tedious to do this with the HUGR API (especially rotations with float parameters).
I've spread this on a couple of lines. Although fully expanding this takes 200+ lines, so not doing that.
the implicit_permutation is just a (mandatory?) field of the tket1 JSON serialisation format. In this case it is the identity, so doesn't serve any purpose.
I've also added a comment and a nice ASCII drawing, courtesy of qiskit's pretty printer.
tests/taso_termination.rs
Outdated
| } | ||
|
|
||
| #[rstest] | ||
| #[ignore = "Takes 800ms"] |
There was a problem hiding this comment.
That doesn't seem like a blocker to me. Even 8s would probably be fine, given it'll take much longer than that to build everything before you've even run a test. 80s, maybe....
There was a problem hiding this comment.
You're right, we can always deactivate this later if need be.
There was a problem hiding this comment.
800ms is not a reasonable test time.
With the latest updates, this test now takes 340ms on CI. This is slower than all the other 52 tests in this crate combined (230ms).
I say we merge this now, but leave the commented-out ignore as a marker in case test times ever become a problem.
|
Sorry if it wasn't clear: this PR merely adds a test, which is the result of me investigating whether TASO returns under controlled conditions. As far as I can tell there is nothing to be fixed, so I have just added the test itself (that now even runs, lol). There is a whole discussion on future work in this area (to which you have already contributed) here: #160 |
# Conflicts: # src/optimiser/taso.rs # src/rewrite/strategy.rs
Closes #152 .
Note: only look at the last commit! Everything else is stuff from other PRs (the API is quite in flux so basing on main would have just created merge conflicts in a moment).