Cirq icon indicating copy to clipboard operation
Cirq copied to clipboard

Ensure scalar parameter names are preserved during conversion

Open natestemen opened this issue 1 year ago • 4 comments

Description: When converting Quil programs with single-value (scalar) parameters, ensure that the parameter name remains consistent by returning the symbolic name.

Motivation: When upgrading from pyquil v3 to v4, the output of a simple program changes subtly. If the starting pyquil program is

program = Program()
theta = program.declare("theta", memory_type="REAL")
program += RZ(theta, 0)

Then the diff between v3 program.out() and v4 program.out() is as follows.

-RZ(theta) 0
+RZ(theta[0]) 0

So far, this has nothing to do with cirq, but when calling cirq_rigetti.circuit_from_quil(program.out()) the instantiated cirq.Circuit now has a parameter name theta_0 which was never specified by the user.

natestemen avatar Oct 08 '24 22:10 natestemen

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Oct 08 '24 22:10 google-cla[bot]

I was able to reproduce this locally, but the CI failure made me look at it again. It seems that declared_size is not always set (i.e. it depends on how you instantiate the pyquil.Program: either via a quil string, or sequentially building it via pyquil operations). This makes it an unreliable signal for determining whether the variable is actually a scalar.

I'm a bit stumped as to what the best approach would be to resolve the underlying issue. Any advice would be appreciated.

natestemen avatar Oct 09 '24 04:10 natestemen

@natestemen - it seems the CLA check is failing (the second comment here). Please fix, otherwise we are not able to consider the PR.

pavoljuhas avatar Oct 09 '24 17:10 pavoljuhas

Ah sorry about that! Didn't realize it was blocking a review. Resolved now.

natestemen avatar Oct 09 '24 18:10 natestemen

@natestemen - after testing this a bit, I would argue the current cirq parameters naming is more close to the pyquil's program.out() behavior in your initial https://github.com/quantumlib/Cirq/pull/6755#issue-2574314732. The declaration of the theta scalar creates an array of size 1 so it's value is later referenced as theta[0].

Unless this is a bug in pyquil I feel it is better to leave the code as is. (cc @jselig-rigetti - can you please comment?)

BTW, the test quil code added in this PR renders the same for pyquil 3 and 4 with parameter expression theta[0]. Converting such program to a circuit with parameter theta_0 sounds sensible.

from pyquil import Program
print(Program("DECLARE theta REAL[1]\nRZ(theta) 0"))

output:

DECLARE theta REAL[1]
RZ(theta[0]) 0

pavoljuhas avatar Dec 04 '24 08:12 pavoljuhas

The main point of confusion is that parameter names are no longer consistent across SDKs. E.g. I define a a pyquil program with a parameter, I have to do some amount of logic to figure out what the parameter name is in the cirq circuit. That's a surprising behavior IMO.

That said, I understand where you're coming from, and the current implementation keeps things simple. I appreciate you taking a look!

natestemen avatar Dec 05 '24 04:12 natestemen