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

CRZ gate decomposition error #103

Closed
anitalu724 opened this issue Dec 12, 2022 · 7 comments
Closed

CRZ gate decomposition error #103

anitalu724 opened this issue Dec 12, 2022 · 7 comments

Comments

@anitalu724
Copy link

Hi, according to pyzx/circuit/gates.py, your decomposition and tensor of crz(pi/2) is shown below:
Screen Shot 2022-12-12 at 5 31 48 PM

However, the correct form of crz(pi/2) should be like this:
Screen Shot 2022-12-12 at 5 37 20 PM

Refer to this transformation:
318993810_684855746624910_6597546469576874782_n (1)

May I submit a PR to modify this bug?

@jvdwetering
Copy link
Collaborator

So a problem here is that different libraries and people use different standards for this gate. I believe I based the current definition on the qasm spec, but I can check

@jvdwetering
Copy link
Collaborator

Yeah, I was right: the official QASM2.0 spec says that the current definition we have is the correct one. See page 12 of https://arxiv.org/pdf/1707.03429.pdf.
What was your intended usecase for the other definition? Your definition is the one that the QASM spec calls cu1 btw.

By the way, I do agree that your definition is more sensible, and it is also how I think of a crz gate. But that is not how it is defined in the spec.

@anitalu724
Copy link
Author

anitalu724 commented Dec 13, 2022

Maybe it's the version of OpenQASM that causes the misunderstanding! According to OpenQASM3.0, the CRZ gate is more likely to be represented as the CU1 gate you've mentioned!

@jvdwetering
Copy link
Collaborator

Yes QASM3.0 seems to use your definition. Which is annoying because now the specs aren't compatible apparently?
I don't really know how to deal with this now.

dlyongemallo added a commit to dlyongemallo/pyzx that referenced this issue Sep 5, 2023
… in pyzx.

The OpenQASM 2 spec defines the following:
```
gate rz(phi) a { u1(phi) a; }
gate crz(lambda) a,b
{
    u1(lambda/2) b;
    cx a,b;
    u1(-lambda/2) b;
    cx a,b;
}
gate cu1(lambda) a,b
{
    u1(lambda/2) a;
    cx a,b;
    u1(-lambda/2) b;
    cx a,b;
    u1(lambda/2) b;
}
```

pyzx's `ZPhase` gate and `CRZ` gates are equivalent to, respectively, OpenQASM 2's `rz` (a synonyn for `u1`) and `crz` gates. Confusingly, the `crz` gate is not simply a controlled version of the `rz` gate; that gate is named `cu1`.

Meanwhile, the OpenQASM 3 spec defines:
```
gate rz(λ) a { gphase(-λ/2); U(0, 0, λ) a; }
gate crz(θ) a, b { ctrl @ rz(θ) a, b; }
```

This has the advantage that `crz` is now simply a controlled version of `rz`, at the expense of changing the meaning of `rz` so that it no longer matches `u1`/`ZPhase`.

This difference in conventions leads to user confusion and coding errors, as seen in issues zxcalc#102 and zxcalc#103. As well, changing the behaviour of `qasmparser` depending on the version of OpenQASM used is a blocker for issue zxcalc#116.
dlyongemallo added a commit to dlyongemallo/pyzx that referenced this issue Sep 5, 2023
… in pyzx.

The OpenQASM 2 spec defines the following:
```
gate rz(phi) a { u1(phi) a; }
gate crz(lambda) a,b
{
    u1(lambda/2) b;
    cx a,b;
    u1(-lambda/2) b;
    cx a,b;
}
gate cu1(lambda) a,b
{
    u1(lambda/2) a;
    cx a,b;
    u1(-lambda/2) b;
    cx a,b;
    u1(lambda/2) b;
}
```

pyzx's `ZPhase` gate and `CRZ` gates are equivalent to, respectively, OpenQASM 2's `rz` (a synonym for `u1`) and `crz` gates. Confusingly, the `crz` gate is not simply a controlled version of the `rz` gate; that gate is named `cu1`.

Meanwhile, the OpenQASM 3 spec defines:
```
gate rz(λ) a { gphase(-λ/2); U(0, 0, λ) a; }
gate crz(θ) a, b { ctrl @ rz(θ) a, b; }
```

This has the advantage that `crz` is now simply a controlled version of `rz`, at the expense of changing the meaning of `rz` so that it no longer matches `u1`/`ZPhase`.

This difference in conventions leads to user confusion and coding errors, as seen in issues zxcalc#102 and zxcalc#103. As well, changing the behaviour of `qasmparser` depending on the version of OpenQASM used is a blocker for issue zxcalc#116.
@dlyongemallo
Copy link
Contributor

The definitions in the OpenQASM 3 header file are:

gate rz(λ) a { gphase(-λ/2); U(0, 0, λ) a; }
gate crz(θ) a, b { ctrl @ rz(θ) a, b; }

If I understand this correctly, crz is actually defined the same between OpenQASM versions 2 and 3 (and also the CRZ gate in pyzx). What actually differs between OpenQASM versions 2 and 3 is the definition of the rz gate. OpenQASM 2 uses what's maybe the more intuitive definition (diag(1, exp(i*phi))), whereas OpenQASM 3's definition quoted above makes it more consistent with crz. But since the CRZ gate is defined in the same way between pyzx and both versions of OpenQASM, I believe this issue can be closed.

@dlyongemallo
Copy link
Contributor

PR #156 specifically adds a test to document the behaviour of rz and crz. I think this issue can be closed.

@dlyongemallo
Copy link
Contributor

This issue can be closed.

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

No branches or pull requests

3 participants