-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add Clifford.from_matrix #9475
Add Clifford.from_matrix #9475
Conversation
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 4676599158
💛 - Coveralls |
Why do you need to add a general method to the Clifford class? and not add specific additional Clifford gates (like: |
# are a single qubit Clifford gate rather than raise an exception. | ||
# If the gate is not in predefined basis gates but with up to 3 qubits, | ||
# we try to construct a Clifford to be appended from its matrix representation. | ||
if isinstance(gate, Gate) and len(qargs) <= 3: |
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.
Hi, a quick question. With the current order of if
-statements, would the new code be executed even for gates that already have a definition? I have not done any experiments, but it feels that constructing a Clifford from a matrix could be slower than constructing a Clifford from the defining quantum circuit.
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.
Good question. I know construction from definition
is faster if possible but it does not always work because the definition
may contain non-Clifford gates. For example, the definition of ECRGate contains RZX(pi/4)
, which is not a Clifford. The definition
of a gate is up to the gate designer, so we cannot fully rely on it. And I just prioritize capability to performance in this commit.
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.
Another suggestion would be to manually define all the relevant gates here:
https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/quantum_info/operators/symplectic/clifford_circuits.py
(you can check both gate name and parameters, and see if they fit one of the known patterns)
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 I understand correctly from the tests, there about 10-15 gates, where theta is an integer product of pi/2 or pi, right?
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.
Thanks! I was merely suggesting to change the order of checks:
if gate.definition is not None:
return _append_circuit(clifford, gate.definition, qargs)
try:
# new code here
raise QiskitError(f"Cannot apply {gate}")
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 we change the order of check, we may need to copy clifford
and when we have any exception, i.e. we fails to all-Clifford decomposition (based on definition
), we need to restore the clifford that was before trying unroll. So the actual code would require more lines, but it's possible, so I'll try that in another branch for comparison.
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.
@alexanderivrii I've tried the implementation that changes the order of check in this branch. I've confirmed that it gets faster by increasing the complexity of the implementation. Do you want to include the change in this PR?
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.
It's generally good to add more gate (like Rz(pi/2)
and u
for angles that are multiples of pi/2
) to clifford_circuits.py
.
My PR #9623 may also be useful. Would you like to add other gates (also with parameters) directly to clifford_circuits.py
?
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.
@ShellyGarion I'm not thinking adding more _append_*
to clifford_circuits.py
is the best approach but it should a option to be considered for improving the performance of Clifford.from_circuit
(see my comment in #9582 for other options). Let's continue to discuss it in #9582.
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.
break | ||
else: | ||
raise QiskitError("Non-Clifford matrix is not convertible") | ||
|
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.
One more quick question. I have not done the math, but I am wondering if it's possible to check whether a given matrix (here U * Xi * Udg
) is close to some Pauli matrix without explicitly going over all of the Pauli matrices?
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.
These lines are using a bit strange grammar of Python: This else
clause is coupled with for
not if
. So they do go over all of the Pauli matrices.
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.
Sorry, I did not highlight the right lines. I believe (but I have not done the math) that the (tensor products of) Pauli matrices are very special unitary matrices, and it should be possible to check whether a given matrix (either U * Xi * Udg
or U * Zi * Udg
) is close to a Pauli matrix without explicitly checking np.close()
against the 2*4^n Pauli matrices.
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.
Yeah, I'm a bit concerned that there may be a more efficient algorithm then current one. If anyone knows such an algorithm, please let me know.
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 believe that if you look at the 2^N x 2^N unitary matrix corresponding to a Pauli matrix, then this unitary matrix must have only one nonzero entry in every row and every column, moreover each nonzero element can only be 1, -1, i or -i. Intuitively, I think this should be enough to work out the exact Pauli.
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.
Great suggestion, thanks! The new algorithm should be
- Check if the
U * Xi * Udg
(orU * Zi * Udg
) has exactly 2^N non-zero (i.e. 1, -1, i or -i) elements (if not, U is non-Clifford) - Reconstruct the Pauli string (a row of the resulting tableau) from the positions and values of the 2^N non-zero elements (if impossible, U is non-Clifford)
Hence, we don't need to create the table with 2^N x 2^N matrices corresponding 4^N Paulis in advance. I'll try the above algorithm.
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.
Thank you! Here are my five cents (and @ikkoham and @ShellyGarion are welcome to correct this):
- If a gate is going to be used a lot, it might be best to provide methods that directly operate on the Clifford tableau (like what is done in
_append_cx
or_append_w
) - However, having a generic fallback mechanism is a great idea, so I think this development is very useful
- If the gate has a
definition
, then it might be more efficient to consider thedefinition
first, before constructing the unitary matrix and converting it to Clifford. On the other hand, it is possible that the default definition of a Clifford gate might contain non-Clifford gates, so the construction based on the definition alone might not be possible
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.
Regarding the more efficient O(4^n) (instead of O(16^n)) algorithm, implemented at bad10de
Excuse me for my short explanation @ShellyGarion . With this, we no longer need to update Another good use case for this new function is construction from a custom gate (suggested by @nkanazawa1989). The previous logic rely on the gate name, so for example when we define a custom cx gate subclassing the standard CXGate, |
I've tried a new algorithm suggested by @alexanderivrii in another branch and confirmed it works well, i.e it's much faster than the naive algorithm currently implemented as expected (see the benchmark results in the link). |
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.
Thanks @itoko , this will be a nice improvement. I think @ShellyGarion 's comment is hinting that another thing that would be nice to have is something like Clifford.from_operation
or similar, that could rely on methods that might be quicker than generating and checking the unitary matrix representation.
For example, checking if the name
is in the list of hard coded names in _BASIS_1Q
/_BASIS_2Q
to know if an Instruction
is Clifford, or utilizing the equivalence library to see if a Gate
has an all Clifford decomposition, or for cases like the parameterized rotations where there may be a simple arithmetic check on the params
to know if it is Clifford.
It doesn't look like this is necessary for this PR though, so I will open a new issue for it.
According to tests we have 3 types of gates to handle:
|
@ShellyGarion I think the above comment is a good starting point of the discussion on #9576 , while I think it is beyond the scope of this PR. Let me focus on the addition of |
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.
Thank you. Great work. The direction looks good to me. Could you implement from_operator
method as well for consistency in quantum_info
.
|
||
|
||
def _n_half_pis(param) -> int: | ||
try: |
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.
How about if not isinstance(param, ParameterExpression)
?
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 need to try cast here because that checks if a ParameterExpression is bounded or not (not checking the type of param
).
@ikkoham I've updated based on the review comment. Could you take a final look? |
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! Perhaps performance could be improved, but I can't think of any idea right now and will leave it open for future work.
@@ -989,13 +1037,58 @@ def test_instruction_name(self, num_qubits): | |||
clifford = random_clifford(num_qubits, seed=777) | |||
self.assertEqual(clifford.to_instruction().name, str(clifford)) | |||
|
|||
def visualize_does_not_throw_error(self): | |||
def test_visualize_does_not_throw_error(self): |
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.
Nice catch!
* Add Clifford.from_matrix * Add tests * Add reno * Faster Clifford.from_matrix O(16^n)->O(4^n) * Add infinite recursion test case * Change to try append with definition first * Add u gate handling for speed * Improve implematation following review comments * Add Clifford.from_operator * Update reno * Lint * more accurate reno --------- Co-authored-by: Ikko Hamamura <[email protected]>
* Add Clifford.from_matrix * Add tests * Add reno * Faster Clifford.from_matrix O(16^n)->O(4^n) * Add infinite recursion test case * Change to try append with definition first * Add u gate handling for speed * Improve implematation following review comments * Add Clifford.from_operator * Update reno * Lint * more accurate reno --------- Co-authored-by: Ikko Hamamura <[email protected]>
Add
Clifford.from_matrix
method that create a Clifford object from Clifford unitary matrix. More importantly, using this method internally,Clifford.__init__
is updated so that it can take anyGate
object up to 3 qubits as long it supportsto_matrix
method, including parameterized gates such asRz(pi/2)
, which were not convertible before. As a result, it fixes an issue in qiskit-experiments.