-
Notifications
You must be signed in to change notification settings - Fork 73
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
Add: sqisw-decompose compiler #868
Conversation
Nice! I’ll try to have a look over the weekend. |
If you wait until next weekend, it might even work! I've got the two sqisw case working but something is amiss in the three sqisw case. I think its that I'm mis-using the output of In any case, any feedback is appreciated! |
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.
Even though you said this isn't finished, I took a look anyway — I'm more likely to have the time this holiday weekend than generally.
So far so good! Some comments. Feel free to ping again as you make progress.
Returns two values L0 and L1, both lists of single-qubit gate applications, acting on q0 and q1 respectively. | ||
L0 is a list of three rotations RZ(γ) RX(α) RZ(θ), and L1 is a list containing one RX(β). | ||
|
||
L(x,y,z) ~ SQiSW [ RZ(γ)RX(α)RZ(γ) ⊗ RX(β)] SQiSW |
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.
theta rather than gamma?
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.
yerp. typo.
q0 and q1 are qubit indices | ||
|
||
Returns two values L0 and L1, both lists of single-qubit gate applications, acting on q0 and q1 respectively. | ||
L0 is a list of three rotations RZ(γ) RX(α) RZ(θ), and L1 is a list containing one RX(β). |
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 haven't looked at the algorithm in the appendix to know if this is really the most natural thing to do, but you should consider returning an anonymous gate application specified by a matrix, so that the outer compiler loop can more effectively "trampoline" it into whatever 1Q native gate set is available.
(Actually looking at the output below, it may well be that what you've / they've written is already the most natural thing to do. Still something to chew on; maybe they worked harder than they should have to produce these formulas, and we can short-circuit that procedure.)
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.
This is something that I really wasn't sure about: to perform the gate compositions myself and spit out anon gates, or to spit out the rotations with explicit angle parameters. Thanks for the take!
Generally, these kinds of "what is best for quilc" questions are what I'm most uncertain about right now.
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.
This specific case can go either way, since the compressor is going to re-do the work I'm suggesting no matter what. But the general philosophy is that individual compiler routines should do as little work + express as few opinions as they can get away with, so that the compiler loop can creatively combine them. The "nativization" loop has more options for what to do with anonymous gates than it does with fixed gate sequences, hence the preference — but, again, the compressor's primary strategy is to re-anonymize sequences in an effort to force the nativizer to find unexpected cancellations, so it'll probably come out the same in the end.
(define-self-documenting-constant 2pi (* 2 pi)) | ||
(define-self-documenting-constant 4pi (* 4 pi))) | ||
(define-self-documenting-constant 4pi (* 4 pi))) |
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.
(nbd, but there's a stray space here before EOL)
|
||
[A0 ⊗ A1] L(x,y,z) [C0 ⊗ C1] SQiSW [B0 ⊗ B1] | ||
|
||
where L(x,y,z) is the canoncal representative of the class for whose |
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.
typo, canonical
Returns 9 values. x y z A0 A1 B0 B1 C0 C1 with the following interpretation. | ||
If x0,y0,z0 are canonical paramters for an arbitrary 2 qubit gate, then that gate is equivalent to | ||
|
||
[A0 ⊗ A1] L(x,y,z) [C0 ⊗ C1] SQiSW [B0 ⊗ B1] |
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.
Say what the types of A*, B*, C* are. I'd have guessed matrices, but looks like they're gate sequences.
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.
Yup, they're lists of gate applications. @stylewarning made the same suggestion.
Also, that composition expression in the docstring is wrong - The C's and B's need to swap positions. Just a typo. Sorry for any confusion while you were reviewing. A corrected docstring will be pushed on Monday.
(b0 b1 can1 a0 a1) (canonical-decomposition instr) | ||
(destructuring-bind | ||
;; making an assumption that I can divide by two here. | ||
;; TODO: check that this isn't bunk. |
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 myself haven't looked, but canonical-decomposition
is where I'd recommend starting your bug search. The coordinates used in PCS, which I think are what's used here, have a goofy normalization condition that causes its canonical gates to span a triangular prism rather than a tetrahedron. If that's indeed what's causing the problem, you could inject an extra step here to re-scissor its output into the tetrahedron you expect — but you would probably make everyone's life easier by changing it to use the same convention everyone else does. I don't think much (anything?) in quilc depends on that alternative convention, and it was definitely a flub on our part not to do that from the start.
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.
More info on this scissors business in Remark 2.7 and all over this Python file.
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.
✂️➡️🔺
(inst b1) | ||
(inst (anon-gate "E1t" (magicl:conjugate-transpose (gate-matrix e1)) q1)) | ||
(inst "SQISW" () q1 q0) | ||
(mapc #'inst c0) |
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 think there's an inst*
. If there isn't, there should be!
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.
GTK. I may add one if there isn't.
|
||
(t | ||
(destructuring-bind | ||
(cx cy cz f0 f1 g0 g1 h0 h1) (canonicalize-for-sqisw x y z q0 q1) |
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.
Here again I'd recommend trampolining rather than duplicating the decomposition above. You can define-compiler
two versions of this routine, version (a) for two 2Q case — it operates only on CAN
gates and has a :where
clause which imposes that coordinate inequality, which the compiler manager will take to mean "this sometimes works and sometimes yields short output, so we'll try it first and not sweat a failure" — and then a second version (b) which handles the "general" case, which emits 1Q * SQiSW * 1Q * CAN * 1Q, where the compiler loop will re-walk this emitted sequence and automatically apply (a) to the emitted CAN.
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.
Yeah I was just thinking about this this morning: Two define compilers, one for the two-sqisw case using a "guard" to check the condition, and then utilizing that compiler in the body of the three-sqisw case's compiler definition.
Thanks for your feedback! I really appreciated it.
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.
Similarly to my other reply-comment, leaning on the outer loop means (1) you don't even need to explicitly call out to the depth 2 case within the depth 3 case, since it'll do it for you; and (2) if someone ever implements an even clever-er inner decomposition, not explicitly calling out to the depth 2 case in this code will leave the compiler loop the flexibility to select its preferred inner decomposition.
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 pushed again. the depth-3 case still is not working. I am going to narrow my search to |
NOTE: I will squash the commits on this branch and do a new PR to eliminate commit noise. In this commit, I have added several functions and tests in comment blocks. These will be removed from the final PR. They are here to aid reviewers by adding information about which questions have been so far addressed, which functions can be counted on, and so on. cheers
adding this before i forget: I imagine that we should call the gate |
This is not ready btw. Even though.. somewhat mysteriously, checks have passed. I should add tests specific to these compiler rules to the suite. |
I'm going to close this PR, flatten commits, and open a new one. |
This branch intends to add sqisw decomposition to quilc, as per #766 .
What remains to be done:
define-compiler
defs for specific known gatesWhat has been done so far: A
define-compiler
definition has been added that decomposes an arbitrary 2 qubit gate into at most three SQiSW gates interleaved by single qubit gates, and, for 80% of all 2 qubit gates, only requires two SQiSW gates.As this is my first attempt to contribute to the compiler, I am looking for critique on anything that is obviously wrong or obviously flouting conventions.