Skip to content
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

Feature: "DEFEXPI" #498

Merged
merged 35 commits into from
Dec 18, 2019
Merged

Feature: "DEFEXPI" #498

merged 35 commits into from
Dec 18, 2019

Conversation

ecpeterson
Copy link
Contributor

@ecpeterson ecpeterson commented Dec 9, 2019

Back at it again.

This PR adds support for:

  • A new gate definition class, DEFGATE ... AS PAULI-SUM, which defines a (possibly parametric) gate as an exponentiated Pauli sum (with possibly parametrically-determined coefficients). For example, the following gives a gate definition for CPHASE:
DEFGATE CPHASE(%alpha) p q AS PAULI-SUM:
    ZZ(-%alpha/4) p q
    Z(%alpha/4) p
    Z(%alpha/4) q
  • Parametric compilation routines for Pauli sum gate definitions which are linear in a single gate parameter.

Some possible follow-ons:

  • Benchmark this against the Tweedledum diagonal compiler.
  • Investigate the diagonal expansion strategy. Can it be made usefully topology-sensitive?
  • Write some tests.

Attendant PRs:

Closes #422. Closes #423.

@ecpeterson ecpeterson requested a review from a team as a code owner December 9, 2019 17:08
Copy link
Member

@notmgsk notmgsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass. I will admit I barely glanced at the parsing code, since it is the least interesting and probably the buggiest part. 😬

src/build-gate.lisp Outdated Show resolved Hide resolved
src/compilers/linear-paulis.lisp Outdated Show resolved Hide resolved
src/compilers/linear-paulis.lisp Outdated Show resolved Hide resolved
src/compilers/linear-paulis.lisp Show resolved Hide resolved
src/gates.lisp Outdated Show resolved Hide resolved
src/matrix-operations.lisp Show resolved Hide resolved
src/parser.lisp Outdated Show resolved Hide resolved
src/parser.lisp Show resolved Hide resolved
src/parser.lisp Show resolved Hide resolved
src/parser.lisp Outdated Show resolved Hide resolved
@jmbr jmbr self-requested a review December 9, 2019 21:15
Copy link
Contributor

@jmbr jmbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick review for the moment. Things are looking good! I miss having some unit tests.

src/ast.lisp Outdated Show resolved Hide resolved
src/ast.lisp Outdated Show resolved Hide resolved
src/compilers/linear-paulis.lisp Outdated Show resolved Hide resolved
src/gates.lisp Outdated Show resolved Hide resolved
@notmgsk
Copy link
Member

notmgsk commented Dec 10, 2019

Um pequeno erro:

(quil:parse-quil "DEFGATE CPHASE(%alpha) p q AS PAULI-SUM:
    ZZ(-%alpha) p q
    Z(%alpha) p
    Z(%alpha) q

")

Copy link
Contributor

@jmbr jmbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Below are some more suggestions that occurred to me while studying the code.

The fact that compilation and, specifically, gate depth is non-deterministic surprises me. For example,

QUIL> (dotimes (i 10) (print (length (parsed-program-executable-code (quilc::process-program (parse-quil "
DEFGATE HAMILTONIAN(%theta) q0 q1 q2 q3 AS PAULI-SUM:
    ZZIZ(%theta) q0 q1 q2 q3

HAMILTONIAN(pi/4) 0 1 2 3") (quilc::lookup-isa-descriptor-for-name "8Q"))))))

59 
55 
58 
58 
62 
56 
59 
61 
57 
55 
nil

produces different gate depths (the number of 2-qubit gates seems to be constant, though). Is this an artifact of qs-compiler?

src/compilers/linear-paulis.lisp Outdated Show resolved Hide resolved
src/compilers/linear-paulis.lisp Show resolved Hide resolved
src/compilers/linear-paulis.lisp Outdated Show resolved Hide resolved
src/compilers/linear-paulis.lisp Outdated Show resolved Hide resolved
src/compilers/linear-paulis.lisp Outdated Show resolved Hide resolved
src/compilers/linear-paulis.lisp Outdated Show resolved Hide resolved
src/compilers/linear-paulis.lisp Outdated Show resolved Hide resolved
@jmbr
Copy link
Contributor

jmbr commented Dec 12, 2019

@jmbr: I'm not sure what's going on with these inflated CZ counts, but it's not in this area of the code:
I'll investigate tomorrow. When I turn *compiler-noise-stream* on, I see that recognize-UCR gets called on the invocation of HAMILTONIAN, which results in a thrice FORKED RZ, which just compiles worse. After demoting RECOGNIZE-UCR further down the global compilers list, this problem goes away.

Thanks for the heads up. That's spooky indeed.

By the way, just the other day I saw a link to a directly relevant compiler debugging technique: http://blog.ezyang.com/2011/06/debugging-compilers-with-optimization-fuel/

@ecpeterson
Copy link
Contributor Author

This is a neat suggestion. We'd have to track down and eliminate the nondeterminism to make it work for us. That's not an unreasonable task, but one would have to think very carefully about the problem we solved in the addresser by injecting randomization.

@stylewarning
Copy link
Member

When I turn *compiler-noise-stream* on, I see that recognize-UCR gets called on the invocation of HAMILTONIAN, which results in a thrice FORKED RZ, which just compiles worse. After demoting RECOGNIZE-UCR further down the global compilers list, this problem goes away.

Can we document this in the global list as an artifact for future visitors?

Copy link
Member

@stylewarning stylewarning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looking good. I'd like to go through the motions of making sure the addition is (1) in the quil spec and (2) doesn't choke the QVM.

cl-quil.asd Outdated Show resolved Hide resolved
src/ast.lisp Show resolved Hide resolved
@@ -54,16 +54,32 @@ EXAMPLE: The Quil line \"CPHASE(pi) 2 3\" corresponds to the S-expression (build

(define-global-counter **anonymous-gate-counter** get-anonymous-gate-counter)

(defun anon-gate (operator matrix qubit &rest qubits)
(defun anon-gate (operator-or-gate gate-or-parameters qubit &rest qubits)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@_@

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, and I'd take suggestions as to how to name these two cases so that the functions can be split apart. The two use cases are:

(anon-gate "DUMMY-NAME" matrix qn ... q0)
(anon-gate parametric-gate-defn parameter-list qn ... q0)

src/build-gate.lisp Outdated Show resolved Hide resolved
src/compilers/linear-paulis.lisp Outdated Show resolved Hide resolved
src/compilers/linear-paulis.lisp Outdated Show resolved Hide resolved
src/gates.lisp Show resolved Hide resolved
src/gates.lisp Show resolved Hide resolved
src/matrix-operations.lisp Show resolved Hide resolved
@@ -391,3 +391,35 @@ as needed so that they are the same size."
(matrix-equality
kroned-ref-mat
(scale-out-matrix-phases kroned-mat kroned-ref-mat))))))

(defun matrix-expt (m s &key hermitian?)
"Computes EXP(M*S). Only works for unitarily diagonalizable matrices M."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we avoiding the bad boi that is magicl.transcendental

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we are. We are calling out to MAGICL, which calls out to LAPACK, but for the diagonalization routines instead of the exponentiation routines.

Importing MAGICL.TRANSCENDENTAL requires the somewhat uncomfortable secondary import of expokit, which we've so far avoided actually using (and which this also wouldn't use).

src/compilers/linear-paulis.lisp Outdated Show resolved Hide resolved
src/compilers/linear-paulis.lisp Outdated Show resolved Hide resolved
src/compilers/linear-paulis.lisp Outdated Show resolved Hide resolved
Copy link
Contributor

@appleby appleby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests would be good but I see that is under the "follow ons" section of the PR description

src/build-gate.lisp Show resolved Hide resolved
src/parser.lisp Show resolved Hide resolved
src/ast.lisp Show resolved Hide resolved
src/build-gate.lisp Outdated Show resolved Hide resolved
src/gates.lisp Show resolved Hide resolved
src/parser.lisp Outdated Show resolved Hide resolved
@ecpeterson
Copy link
Contributor Author

Added a test, which also fixed a bug introduced during PR review.

Copy link
Contributor

@braised-babbage braised-babbage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's difficult to really verify the details on much of this (e.g. whether the compilation routines are correct, and so on), so one must rely on tests. In that regard, I think testing the output of make-matrix-from-quil before and after compiler-hook on exponentials of random Pauli terms is a nice idea, and gives some confidence. However, for legibility/hackability I would strongly prefer for a few additional tests which are more explicit and which exercise narrower code paths. In particular,

  • pauli-term->matrix seems to basically be doing the equivalent of reducing Pauli factors via magicl:kron, so a test could check that pauli-term->matrix is in fact equivalent to something simpler but perhaps slower
  • parametric-pauli-compiler could have a few explicit sanity checks. For example, exp(-iZt/2) should be equivalent to RZ(t), and so on (might consider doing one for a Pauli term of degree 2 as well).
  • I'm not totally clear on what parametric-diagonal-compiler does but it could probably also be tested explicitly in some simple cases.

src/parser.lisp Show resolved Hide resolved
@jmbr
Copy link
Contributor

jmbr commented Dec 18, 2019

It's difficult to really verify the details on much of this (e.g. whether the compilation routines are correct, and so on), so one must rely on tests. In that regard, I think testing the output of make-matrix-from-quil before and after compiler-hook on exponentials of random Pauli terms is a nice idea, and gives some confidence. However, for legibility/hackability I would strongly prefer for a few additional tests which are more explicit and which exercise narrower code paths. In particular,

I concur. The exp-pauli function (and its corresponding unit test) could provide a baseline for comparison.

@stylewarning stylewarning merged commit 84f7cbf into master Dec 18, 2019
@stylewarning stylewarning deleted the feature/defexpi branch December 18, 2019 19:07
@stylewarning
Copy link
Member

@ecpeterson please make sure any follow ons are filed as issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants