-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Create a gate to represent a PauliRotation #6598
Comments
cc: @pavoljuhas |
I dived into this as part of Unitaryhack. A fix of
While one could expect a 4x4 unitary describing The general behavior is that The good news is that
At this point three options to close this:
|
@burlemarxiste thanks for the invisitigation. I assigned the task to you. can you try to modify the methods in DensePauliString to behave correctly (i.e. not drop identity terms). hepefully nothing breaks. if lots of things break then we may accept a patch solution that converts DensePauliString to PauliStringPhasorGate when needed. but we should first try to make DensePauliString work |
Thanks! I actually found a flaw in my reasoning and The documentation states that "When applied on qubits, a DensePauliString results in cirq.PauliString as an operation". Any insight on the original reason for this behavior, since it now needs to be fixed? |
@burlemarxiste no but I presume there was a good reason. anyway that can't be changed now since that will most likely break lots of things, but we can add an optional parameter to change the behaviour Cirq/cirq-core/cirq/ops/dense_pauli_string.py Lines 326 to 327 in 7df83d7
you can change the definition of def on(self, *qubits: 'cirq.Qid', sparse: bool=True) : so that the current behaviour is preserved and the same time we can use |
After several red herrings in the internals of The current implementation of
I would thus suggest the following contributions:
I can create issues targeting these specific changes. Some examples of "solutions" I tried to the proposed challenge of getting the right unitary to display:
Both feel like bloat for a very minor benefit. Overall the entire infrastructure around |
Is it worth considering adding I as a proper Pauli gate? The way they silently get removed when working with Pauli strings has come up before, and the asymmetry can be hard to work with. #5715. We have optimizers now that can explicitly remove them when desired, so the implicit behavior isn't needed. It would be a fairly large change and there may be good reasons not to do it regardless (idk). But figured I'd throw it out there. |
Current status:
main...burlemarxiste:Cirq:main At this stage, the only remaining quirk is that One should decide if displaying a unitary for the whole space (as opposed to only the non-trivial subspace) is worth bloating A refactoring of |
@burlemarxiste I think you are getting side tracked ... try to follow the logic that happens when creating a PauliSum from a DensePauliString ... find the place where the identity gets dropped and propose a solution for that |
Yes, I tried to follow that logic, see my first message. To summarize:
The superficial solution for all this is to allow My point is that even with this change, we'll still get incorrect results or odd corner cases because To me, having a correctly working Could you please answer the following questions:
Why are the second and third unitary identical to the first?
The docstring for Whether it helps or not with issue 6598, shouldn't we fix this?
|
I did a hacky solution that only affect >>> cirq.PauliSum.from_pauli_strings(cirq.DensePauliString('XI')(*cirq.LineQubit.range(2)))
cirq.PauliSum(cirq.LinearDict({frozenset({(cirq.LineQubit(0), cirq.X), (cirq.LineQubit(1), (cirq.X**0.0))}): (1+0j)}))
>>> cirq.DensePauliString('XI')(*cirq.LineQubit.range(2))
((1+0j)*cirq.X(cirq.LineQubit(0))*(cirq.X**0.0).on(cirq.LineQubit(1)))
>>> cirq.PauliSum.from_pauli_strings(cirq.DensePauliString('XI')(*cirq.LineQubit.range(2)))
cirq.PauliSum(cirq.LinearDict({frozenset({(cirq.LineQubit(0), cirq.X), (cirq.LineQubit(1), (cirq.X**0.0))}): (1+0j)}))
>>> cirq.unitary(cirq.PauliSum.from_pauli_strings(cirq.DensePauliString('XI')(*cirq.LineQubit.range(2)))).round(3)
array([[0.+0.j, 0.+0.j, 1.+0.j, 0.+0.j],
[0.+0.j, 0.+0.j, 0.+0.j, 1.+0.j],
[1.+0.j, 0.+0.j, 0.+0.j, 0.+0.j],
[0.+0.j, 1.+0.j, 0.+0.j, 0.+0.j]])
>>> cirq.PauliSumExponential(cirq.DensePauliString('XI')(*cirq.LineQubit.range(2)), exponent=np.pi/4)
cirq.PauliSumExponential(cirq.PauliSum(cirq.LinearDict({frozenset({(cirq.LineQubit(0), cirq.X), (cirq.LineQubit(1), (cirq.X**0.0))}): (1+0j)})), 0.7853981633974483)
>>> cirq.unitary(cirq.PauliSumExponential(cirq.DensePauliString('XI')(*cirq.LineQubit.range(2)), exponent=np.pi/4)).round(3)
array([[0.707+0.j , 0. +0.j , 0. +0.j , 0. +0.707j],
[0. +0.j , 0.707+0.j , 0. +0.707j, 0. +0.j ],
[0. +0.j , 0. +0.707j, 0.707+0.j , 0. +0.j ],
[0. +0.707j, 0. +0.j , 0. +0.j , 0.707+0.j ]]) whether there is an issue with note casting identity as X**0 is not the desired solution it's just a hacky way to show that densepaulistring is all that is needed for this issue.
I think: yes |
I genuinely thought I did the right thing by reporting and documenting issues in a closely related part of the code, and by avoiding modifying code in a way that pushes it outside of what it seems to have been designed for. From my understanding, If I had the power to do so, I would cautiously flag this class as "needs agreed design". But I don't. I thank you for the patience you have shown in this, and for having relieved me of it. |
@burlemarxiste I unassigned the issue because we got communcitation from the hackathon organizers not to assign tasks until they are completed.. that's per the hackathon rules it should be a race for who fixes the issue first, I also unassigned the other issues that we had that have not been resolved it ... so feel free to continue working on it if you want. sorry for the misunderstanding you can also pick another task if you feel this task is too hard |
This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days |
This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days |
This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days |
Is your feature request related to a use case or problem? Please describe.$e^{i \theta P}$ where $P$ is a pauli string, appears a lot in discussions around rotations and magic states. Cirq has a a general way for representing exponentials of pauli strings $e^{i \theta \sum_k P_k}$ called PauliSumExponential however the pauli strings are stored as sparse strings (i.e. dropping identitiy operations) which results in the wrong unitary.
The operation
Example$\theta = \frac{\pi}{4}$ correct unitary should be
P=XI
but cirq gives
Describe the solution you'd like
ideally a fix to the PauliSumExponential operation or a new gate for representing the exponential of one pauli string
What is the urgency from your perspective for this issue? Is it blocking important work?
P2 - we should do it in the next couple of quarters
The text was updated successfully, but these errors were encountered: