-
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
MatrixGate names don't survive serialization #6026
Conversation
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 a minor comment.
cirq-core/cirq/ops/matrix_gates.py
Outdated
@@ -114,11 +114,11 @@ def with_name(self, name: str) -> 'MatrixGate': | |||
return MatrixGate(self._matrix, name=name, qid_shape=self._qid_shape, unitary_check=False) | |||
|
|||
def _json_dict_(self) -> Dict[str, Any]: | |||
return {'matrix': self._matrix.tolist(), 'qid_shape': self._qid_shape} | |||
return {'matrix': self._matrix.tolist(), 'qid_shape': self._qid_shape, **({'name': self._name} if self._name else {})} |
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.
Strictly speaking this sends empty string to None
after serialization+deserialization.
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 an empty string as a valid name? If so, I’ll update the check to more explicitly check None.
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.
Well, at present the empty string is accepted by the ctor, so let's make sure it works in serde, too.
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. I've updated the check logic and added an empty name test.
fixes #637 (this was broken by #626, but didn't show up because quantumlib/Cirq#6026 wasn't merged into non-dev cirq until version 1.2.0)
Closes #5999