-
Notifications
You must be signed in to change notification settings - Fork 982
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
Rename _channel_
to _kraus_
#4139
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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.
Do we consider _channel_
to be part of our API? I know the preceeding underscore means "private" in Python, but in Cirq (and IPython, numpy(?), maybe others) flanking underscores means magic method that 3rd party classes can implement.
I can certainly see a situation where someone has their own gate or channel that defines _channel_
and this would break them. Would it be worth the effort to go through a deprecation cycle? I don't think it would be too hard to fall back to _channel_
(and emit a warning) if it exists and _kraus_
doesn't.
cirq-core/cirq/protocols/channel.py
Outdated
def _channel_(self) -> Union[Sequence[np.ndarray], NotImplementedType]: | ||
r"""A list of matrices describing the quantum channel. | ||
def _kraus_(self) -> Union[Sequence[np.ndarray], NotImplementedType]: | ||
r"""A list of Kraus operators describing the quantum channel. |
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.
kraus 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.
Done.
@@ -266,7 +266,7 @@ def err_str(buf_num_str): | |||
return default | |||
raise TypeError( | |||
"object of type '{}' has no _apply_channel_, _apply_unitary_, " |
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.
Should we also rename _apply_channel_
, or is that method more generally applicable to other channel representations?
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 think we should make sure that apply_channel protocol works with all channel representations. It's more user-friendly than having an apply* protocol for each representation and forcing the user to check which representation they have at their disposal.
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.
Looks good to me - I think adding _channel_
back into the interface resolves both Matt's comment and mine.
Follow-up to quantumlib#4139 and quantumlib#4195. Fixes quantumlib#4138.
This renames magic methods
_channel_
to_kraus_
and_has_channel_
to_has_kraus_
. Follow-up renames the protocolscirq.channel
tocirq.kraus
andcirq.has_channel
tocirq.has_kraus
.Context: #4138.