-
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
Operator equality with (clean) ancillas #12968
Operator equality with (clean) ancillas #12968
Conversation
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 10471646092Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
As @Cryoris suggested offline, it is better to make this function private for now. So I have prefixed it with the underscore and removed the lovingly-created release notes. |
ancilla_qubits: a list of clean ancilla qubits. | ||
rtol (float): relative tolerance value for comparison. | ||
atol (float): absolute tolerance value for comparison. | ||
ignore_phase (bool): ignore complex-phase difference between 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.
Change the order to match the actual args order
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 point, done in 4de623a.
ignore_phase (bool): ignore complex-phase difference between matrices. | ||
|
||
Returns: | ||
bool: True if operators are equal up to clean ancilla qubits. |
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.
bool: True if operators are equal up to clean ancilla qubits. | |
bool: True iff operators are equal up to clean ancilla qubits. |
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.
Done in 4de623a.
def _equal_with_ancillas( | ||
self, | ||
other: Operator, | ||
ancilla_qubits: list, |
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.
Maybe List[int] instead of list?
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.
Done in 4de623a (well, list[int]
, which I believe is now a bit more preferred).
Returns: | ||
bool: True if operators are equal up to clean ancilla qubits. | ||
""" | ||
if self.dim != other.dim: |
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.
Add type checking on other
similarly to how it's done in equiv()
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.
The equiv
code does more than just type-checking: if converts other
to an Operator
when it's not already, so for example one can check the equivalence of an Operator
and a QuantumCircuit
. I don't think this fully makes sense for this (currently private) function.
for q in range(num_qubits): | ||
if q in ancilla_qubits: | ||
pattern[pos] = q | ||
pos += 1 |
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.
Might be a nitpicking, but I disliked the loop duplication up to the if condition ...
So to alleviate this and hopefully also improve readability, consider something like this:
pattern = []
ancillas = []
for q in range(num_qubits):
if q in ancilla_qubits:
ancillas.append(q)
else:
pattern.append(q)
pattern += ancillas
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.
That's a great suggestion, done in 4de623a.
Thanks @Cryoris and @eliarbel for the reviews! I haven't fully understood Julien's comment the first time: as the new function is not yet used anywhere in the main Qiskit code, for now it's better to make it a private function and not have it in Eli, I have addressed your review comments in 4de623a. |
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
I approve for merge, though I'm not 100% happy with adding a new file ('operator_utils.py') just for this, knowing that there is a chance that '_equiv_with_ancillas' would become public and even part of Operator. Since there are already _ methods in Operator, roughly in the same vein like _multiply
and _add
I would prefer to keep '_equiv_with_ancillas' in Operator and spare an additional new file in the module.
|
Summary
This PR adds a new method
equal_with_ancillas
to theOperator
class that checks whether the two operators are equal in the subspace where the ancilla qubits are zero.While this is not immediately required for #12961 and #12911, it allows to check correctness of various synthesis methods and transformations in the presence of clean ancilla qubits. (The dirty ancilla qubits should be interpreted as regular qubits and don't matter).
Details and comments
Many thanks to @ShellyGarion for the idea how to implement this in a simple way.
Here is an example (which also appears in the release notes):
The first assertion checks that the two circuits are not equal, and they are not -- due to the CX gates from qubit 1 to other qubits. However, the two circuits are equal if qubit 1 is initially zero, then the CX gates do not matter.