-
Notifications
You must be signed in to change notification settings - Fork 160
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
An extended RZZ pass for unbounded parameters #2072
base: main
Are you sure you want to change the base?
Conversation
All the bugs have been fixed, it's not in draft mode anymore but ready for review. |
Is there a risk that the pass will run again and again, forever? |
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 very clever, Yael and Liran.
I think the concern about running forever is coming from the use of the modified
variable in a loop? That variable is just used to determine whether to reuse the original data or new data and not to restart the loop, so I think it is okay.
I have a couple concerns about this PR, but they may not be issues:
-
Should we be concerned about the blowing up in size of the input circuit?
The simple circuit with a single rzz with angle
p
goes from having a data attribute like:[CircuitInstruction(operation=Instruction(name='rzz', num_qubits=2, num_clbits=0, params=[Parameter(p)]), qubits=(Qubit(QuantumRegister(2, 'q'), 0), Qubit(QuantumRegister(2, 'q'), 1)), clbits=())]
To one like:
[CircuitInstruction(operation=Instruction(name='global_phase', num_qubits=0, num_clbits=0, params=[ParameterExpression(1.5707963267949*(1 - sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5)))*((sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 1.5707963267949) + 1)*sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 1.5707963267949)**2/2 - sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 1.5707963267949)**2 + 1)*sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5))**2 + 1.5707963267949*(sign(-p + 12.5663706143592*floor(0.0795774715459477*p) + 9.42477796076938) + 1)*((sign(p - 12.5663706143592*floor(0.0795774715459477*p) - 3.14159265358979) + 1)*sign(p - 12.5663706143592*floor(0.0795774715459477*p) - 3.14159265358979)**2/2 - sign(p - 12.5663706143592*floor(0.0795774715459477*p) - 3.14159265358979)**2 + 1)*sign(-p + 12.5663706143592*floor(0.0795774715459477*p) + 9.42477796076938)**2 - 0.785398163397448*(sign(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949) + 1)*((sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979) + 1)*sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979)**2/2 - sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979)**2 + 1)*sign(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949)**2 - 0.785398163397448*(sign(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979) + 1)*((sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949) + 1)*sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949)**2/2 - sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949)**2 + 1)*sign(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979)**2)]), qubits=(), clbits=()), CircuitInstruction(operation=Instruction(name='rz', num_qubits=1, num_clbits=0, params=[ParameterExpression(1.5707963267949*(sign(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949) + 1)*((sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979) + 1)*sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979)**2/2 - sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979)**2 + 1)*sign(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949)**2 + 1.5707963267949*(sign(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979) + 1)*((sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949) + 1)*sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949)**2/2 - sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949)**2 + 1)*sign(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979)**2)]), qubits=(Qubit(QuantumRegister(2, 'q'), 0),), clbits=()), CircuitInstruction(operation=Instruction(name='rx', num_qubits=1, num_clbits=0, params=[ParameterExpression(1.5707963267949*(1 - sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5)))*((sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 1.5707963267949) + 1)*sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 1.5707963267949)**2/2 - sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 1.5707963267949)**2 + 1)*sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5))**2 + 1.5707963267949*(sign(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979) + 1)*((sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949) + 1)*sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949)**2/2 - sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949)**2 + 1)*sign(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979)**2)]), qubits=(Qubit(QuantumRegister(2, 'q'), 0),), clbits=()), CircuitInstruction(operation=Instruction(name='rz', num_qubits=1, num_clbits=0, params=[ParameterExpression(1.5707963267949*(sign(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949) + 1)*((sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979) + 1)*sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979)**2/2 - sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979)**2 + 1)*sign(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949)**2 + 1.5707963267949*(sign(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979) + 1)*((sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949) + 1)*sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949)**2/2 - sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949)**2 + 1)*sign(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979)**2)]), qubits=(Qubit(QuantumRegister(2, 'q'), 1),), clbits=()), CircuitInstruction(operation=Instruction(name='rzz', num_qubits=2, num_clbits=0, params=[ParameterExpression(-(1 - sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5)))*(p - 6.28318530717959*floor(0.159154943091895*p + 0.5))*((sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 1.5707963267949) + 1)*sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 1.5707963267949)**2/2 - sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 1.5707963267949)**2 + 1)*sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5))**2/2 + (p - 6.28318530717959*floor(0.159154943091895*p + 0.5))*(sign(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) + 1.5707963267949) + 1)*((sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5)) + 1)*sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5))**2/2 - sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5))**2 + 1)*sign(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) + 1.5707963267949)**2/2 + (sign(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949) + 1)*(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979)*((sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979) + 1)*sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979)**2/2 - sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979)**2 + 1)*sign(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949)**2/2 + (sign(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979) + 1)*(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979)*((sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949) + 1)*sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949)**2/2 - sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949)**2 + 1)*sign(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979)**2/2)]), qubits=(Qubit(QuantumRegister(2, 'q'), 0), Qubit(QuantumRegister(2, 'q'), 1)), clbits=()), CircuitInstruction(operation=Instruction(name='rx', num_qubits=1, num_clbits=0, params=[ParameterExpression(1.5707963267949*(1 - sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5)))*((sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 1.5707963267949) + 1)*sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 1.5707963267949)**2/2 - sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) + 1.5707963267949)**2 + 1)*sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5))**2 + 1.5707963267949*(sign(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979) + 1)*((sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949) + 1)*sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949)**2/2 - sign(p - 6.28318530717959*floor(0.159154943091895*p + 0.5) - 1.5707963267949)**2 + 1)*sign(-p + 6.28318530717959*floor(0.159154943091895*p + 0.5) + 3.14159265358979)**2)]), qubits=(Qubit(QuantumRegister(2, 'q'), 0),), clbits=())]
Perhaps in the weighing of pros and cons, this large expression is better than not being able to use parametrized circuits with rzz or needing to wrap parameter values in some external method.
-
Do we need to be careful about adding a link between the rzz gate and the rx gate? In practice, they were added at the same time and so rx is always present when rzz is. The first version of the pass just added an assumption of there being an
XGate
though, notRXGate
. Perhaps it is fine the way it is. Or should there be a check that the backend supportsRXGate
as well to be safe? -
I wonder if the parameterized and numerical code paths should be more aligned? Perhaps not, things are readable the way they are. The parameterized path uses a single function adding on parts as it goes while the numerical one has four separate functions. The parameterized version doesn't have the option of splitting up like the numerical one does.
One minor thing -- this is not related to this PR, but while looking at it I noticed the circuit drawings in the quad method docstrings are not aligned properly. Perhaps that could be incidentally fixed here as well.
For (2), I expect other translation passes to translate For (1), I'm more concerned than you, to the point that I estimate that this will PR eventually will not be able to merge, because of the circuit size. This is certainly something that must be checked. |
So the pass managers are just lists of passes that get to run sequentially, not involving loops and fixed points. I suggest at minimum to check if |
@@ -46,7 +46,7 @@ class FoldRzzAngle(TransformationPass): | |||
with angle of arbitrary real numbers. | |||
""" | |||
|
|||
def __init__(self, target: Optional[Union[Target, list[str]]] = None): | |||
def __init__(self, target: Optional[Union[Target, List[str]]] = None): |
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.
If you prefer, you can use from __future__ import annotations
at the top of the import statements and then the new style will work for type hints. Doing this causes the type hints to be converted into strings instead of being evaluated as objects (so it doesn't make the list[...]
syntax actually work on Python 3.9; if you try to use it outside of an annotation, there will be an error).
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 don't mind, do you have a preference?
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.
We are inconsistent enough in this repo that I don't think it matters much in individual PRs like this, it would be more effective to have a PR that unifies across the repo and then we start being strict. My preference is to eventually settle on the new-style, in which case this would be Target | list[str] | None
.
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 don't have a strong preference though I do think it is nice to avoid the typing imports when possible. I mainly wanted to point the option out since I saw the commit message "old style typing required by CI".
Thanks for writing this example out explicitly, @wshanks . I don't have any feedback at the moment, just a question and some comments. First of all, I haven't yet looked at the implementation or any math notes, but based on the gates in the printout, I assume this is implementing something the following case logic (?):
If so, I guess it's not surprising that implementing case logic with parameter expressions end up rather complicated. If there were many thousands of these inside of an input, it would cause bottlenecks various places in the stack. To me, one of the main sticking points is where and how the user opts-in to the two extra rx gates. |
The logic is described in a clear manner at the doc strings of the methods |
Closes #2032.
The transpiler pass of #2043 is extended to accommodate unbounded parameters, including parameter expressions.
Many thanks to Liran Shirizly for the idea.
The tests are currently failing - I'm investigating - opening in draft mode.