-
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
Deprecate PauliTransform #5498
Deprecate PauliTransform #5498
Conversation
- PauliTransform is only used with SingleQubitCliffordGate, is not easily extensible for multi-qubit gates, and has been obsoleted by DensePauliString Fixes: quantumlib#4088
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 question
cirq-core/cirq/ops/clifford_gate.py
Outdated
if x_to is None or z_to is None: | ||
return None | ||
return SingleQubitCliffordGate.from_clifford_tableau( | ||
_to_clifford_tableau(x_to=x_to, z_to=z_to) | ||
) | ||
|
||
def transform(self, pauli: Pauli) -> PauliTransform: | ||
def transform(self, pauli: Pauli) -> Tuple[Pauli, bool]: |
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 return type is a bit odd. Should it be a dense pauli string?
Also is this a breaking change?
Probably should at least be documented.
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.
Making this into a DensePauliString introduces all sorts of circular dependencies, since DensePauliString depends on SingleQubitCliffordGate, and it is tricky to work around this. I could do this if we have inline imports though.
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 changed this a bit. I deprecated transform() but kept the return type the same. I then added two functions (pauli_tuple()) which is the named tuple without the name, and dense_pauli_string which is the pauli_string). What do you think?
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.
Much better.
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.
My only thought is that after deprecation, when we remove PauliTransform it might make sense to still use it or a named tuple, as these [0] and [1]s are a bit hard to read. You could do that now, or you could wait, since it doesn't impact the public API either is good by me.
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 with one question about how you want to handle the fix after deprecation.
- PauliTransform is only used with SingleQubitCliffordGate, is not easily extensible for multi-qubit gates, and has been obsoleted by DensePauliString Fixes: quantumlib#4088
- PauliTransform is only used with SingleQubitCliffordGate, is not easily extensible for multi-qubit gates, and has been obsoleted by DensePauliString Fixes: quantumlib#4088
is not easily extensible for multi-qubit gates, and has
been obsoleted by DensePauliString
Fixes: #4088