-
Notifications
You must be signed in to change notification settings - Fork 984
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
Multiple Qubits Clifford Gate #4791
Conversation
This reverts commit b2d9269.
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 looks great. Just a few cleanup items then we should be good to merge. Also it might be a good idea to split this out into a new module since clifford_gate.py
is starting to get pretty big.
|
||
|
||
@value.value_equality | ||
class CliffordGate(raw_types.Gate, CommonCliffordGates): |
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.
Nit: We should rename this to something like MultiQubitCliffordGate
we already have SingleQubitCliffordGate
. This could be confusing because a user might think that CliffordGate
is a superclass of SingleQubitCliffordGate
which it is not.
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.
Renamed to MultiQubitCliffordGate
for now. But, in my opinion, it is better to have CliffordGate
as base class and make SingleQubitCliffordGate
as derived class in future. (Need more work, which should not be done in this PR anyway). Because SingleQubitCliffordGate
can reuse all CliffordGate
functions and create more functions, like related to bloch rotation, that is unique in the Single Qubit case. (And overwrite some function since in one qubit there can be more efficient implementation?) Idea 2: Alternatively, create an abstract CliffordGate
superclass and move the common methods in SingleQubit and MultipleQubit into the superclass. Wdyt?
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.
But, in my opinion, it is better to have CliffordGate as base class and make SingleQubitCliffordGate as derived class in future
This sounds like a good idea to investigate in a future PR. Should we revert back to CliffordGate to make this follow up PR easier ?
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.
Yes, as discussed in the Cirq-Cynq, will make a follow-up PR to make the SingleQubitCliffordGate as the derived class of CliffordGate. So rename it back.
cirq-core/cirq/ops/clifford_gate.py
Outdated
def _generate_clifford_from_known_gate( | ||
cls, num_qubits: int, gate: raw_types.Gate | ||
) -> 'CliffordGate': | ||
from cirq import LineQubit, sim |
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.
Nit: can we use module imports here and wrap in a lazyloader at the top of the module if nescessary.
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.
Yep, it is working
cirq-core/cirq/ops/clifford_gate.py
Outdated
cls, operations: Sequence[raw_types.Operation], qubit_order: Sequence[raw_types.Qid] | ||
) -> 'CliffordGate': | ||
"""Construct a new Clifford gates from several known operations.""" | ||
from cirq.sim import clifford |
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.
Nit: move to top with lazyload and module import if need be.
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.
Moved. But I am not sure if I understand your meaning correctly?
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.
For this and other imports where I mentioned move to the top I was thinking of using something like this:
# Lazy imports to break circular dependencies.
circuits = LazyLoader("circuits", globals(), "cirq.circuits.circuit")
op_tree = LazyLoader("op_tree", globals(), "cirq.ops.op_tree")
text_diagram_drawer = LazyLoader(
"text_diagram_drawer", globals(), "cirq.circuits.text_diagram_drawer"
)
For a concrete example see moments.py
.
Does this approach also not work ?
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.
Oh, yes! I missed that part. I vaguely remember it has been mentioned to resolve the circular import issue in sync meeting but forgot to call LazyLoader. Yes, it is working!
@MichaelBroughton Thanks for the reviewing! I think I have addressed most comments but also left a few more to discuss. PTAL. |
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.
LGTM, be sure to tag me on the next PR the combines these two gates into an inheritance hierarchy :).
Add initial Clifford Gate with multiple qubits. Compared with SingleQubitCliffordGate, it has fewer functionalities since we cannot enumerate all of them with PauliGates and several special single qubit properties like Bloch rotation no longer exist. Anyway, it provides several basic interactions: 1. It uses Clifford tableau as underlying data representation (different from the state representation). 2. It can be constructed from a tableau or list of operations (`_has_stabilizer_effect_` only). All Clifford gates can be built through \{S, H, CNOT\}, so we can construct any Clifford Gate from the list of operations. We just cannot pre-define it. 3. Decomposing into several basic operations. 4. Get unitary matrix through decomposing (we cannot do this in a reverse way from unitary to Clifford gate :( ). 5. Know how to interact with ActOnCliffordTableauArgs, i.e. it should be able to use with CliffordTableau simulator (Looks like we don't have that in cirq yet? @daxfohl will add that? see quantumlib#4639 and quantumlib#4748.). This PR is part of efforts for quantumlib#3639. Context: this PR doesn't introduce any new algorithms but the key methods are already implemented in quantumlib#4183 and quantumlib#4096.
This PR mainly moved the `SingleQubitCliffordGate` as the derived class of `CliffordGate`. It didn't add any new functionalities or features. The code change looks large but actually not. Because I moved the `SingleQubitCliffordGate` to the place after `CliffordGate` and auto-diff is not that smart enough to tell that. Context #4791. Main change are: 1. Modify the value equality so that SingleQubitCliffordGate can be evaluate equal to the same CliffordGate 2. Moved the module-level initialization common SingleQubitCliffordGate (like X, Y, Z) to the class-level one. 3. Make a few function `SingleQubitCliffordGate` to use `CliffordGate` one.
This PR mainly moved the `SingleQubitCliffordGate` as the derived class of `CliffordGate`. It didn't add any new functionalities or features. The code change looks large but actually not. Because I moved the `SingleQubitCliffordGate` to the place after `CliffordGate` and auto-diff is not that smart enough to tell that. Context quantumlib#4791. Main change are: 1. Modify the value equality so that SingleQubitCliffordGate can be evaluate equal to the same CliffordGate 2. Moved the module-level initialization common SingleQubitCliffordGate (like X, Y, Z) to the class-level one. 3. Make a few function `SingleQubitCliffordGate` to use `CliffordGate` one.
Add initial Clifford Gate with multiple qubits. Compared with SingleQubitCliffordGate, it has fewer functionalities since we cannot enumerate all of them with PauliGates and several special single qubit properties like Bloch rotation no longer exist. Anyway, it provides several basic interactions: 1. It uses Clifford tableau as underlying data representation (different from the state representation). 2. It can be constructed from a tableau or list of operations (`_has_stabilizer_effect_` only). All Clifford gates can be built through \{S, H, CNOT\}, so we can construct any Clifford Gate from the list of operations. We just cannot pre-define it. 3. Decomposing into several basic operations. 4. Get unitary matrix through decomposing (we cannot do this in a reverse way from unitary to Clifford gate :( ). 5. Know how to interact with ActOnCliffordTableauArgs, i.e. it should be able to use with CliffordTableau simulator (Looks like we don't have that in cirq yet? @daxfohl will add that? see quantumlib#4639 and quantumlib#4748.). This PR is part of efforts for quantumlib#3639. Context: this PR doesn't introduce any new algorithms but the key methods are already implemented in quantumlib#4183 and quantumlib#4096.
This PR mainly moved the `SingleQubitCliffordGate` as the derived class of `CliffordGate`. It didn't add any new functionalities or features. The code change looks large but actually not. Because I moved the `SingleQubitCliffordGate` to the place after `CliffordGate` and auto-diff is not that smart enough to tell that. Context quantumlib#4791. Main change are: 1. Modify the value equality so that SingleQubitCliffordGate can be evaluate equal to the same CliffordGate 2. Moved the module-level initialization common SingleQubitCliffordGate (like X, Y, Z) to the class-level one. 3. Make a few function `SingleQubitCliffordGate` to use `CliffordGate` one.
Add initial Clifford Gate with multiple qubits. Compared with SingleQubitCliffordGate, it has fewer functionalities since we cannot enumerate all of them with PauliGates and several special single qubit properties like Bloch rotation no longer exist. Anyway, it provides several basic interactions:
_has_stabilizer_effect_
only). All Clifford gates can be built through {S, H, CNOT}, so we can construct any Clifford Gate from the list of operations. We just cannot pre-define it.This PR is part of efforts for #3639. Context: this PR doesn't introduce any new algorithms but the key methods are already implemented in #4183 and #4096.