-
Notifications
You must be signed in to change notification settings - Fork 990
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 TwoQubitDiagonalGate #2891
Add TwoQubitDiagonalGate #2891
Conversation
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
cc8ff4d
to
fa7fed9
Compare
Hey @cduck, @hanusaj will be taking this implementation from me. |
@hanusaj The Travis tests are failing. |
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
@hanusaj You'll need to squash my commits and commit them as your own. |
7192a41
to
63bd918
Compare
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
73a1e38
to
47bf076
Compare
84de6da
to
cb06057
Compare
3aaf0c9
to
239246c
Compare
8244669
to
17cd6cd
Compare
format Respond to reviews revetr revert again ignore global phase lint Fixed edits for the Add TwoQubitDiagonalGate. Change the docstring for the two_qubit_diagonal_gate.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.
cirq/ops/two_qubit_diagonal_gate.py
Outdated
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
"""Creates the Gate instrance for a two qubit diagonal 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.
lower case 'g` for 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.
Has been addressed.
cirq/ops/two_qubit_diagonal_gate.py
Outdated
# limitations under the License. | ||
"""Creates the Gate instrance for a two qubit diagonal gate. | ||
|
||
The Gate is used to create a 4x4 matrix with the diagonal elements |
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.
lower case g for 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.
Has been addressed.
cirq/ops/two_qubit_diagonal_gate.py
Outdated
# limitations under the License. | ||
"""Creates the Gate instrance for a two qubit diagonal gate. | ||
|
||
The Gate is used to create a 4x4 matrix with the diagonal elements |
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.
Use latex notation for multiplication sign.
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.
Has been addressed.
cirq/ops/two_qubit_diagonal_gate.py
Outdated
|
||
@value.value_equality() | ||
class TwoQubitDiagonalGate(gate_features.TwoQubitGate): | ||
"""A gate given by a diagonal 4x4 matrix.""" |
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.
Use latex notation for multiplication sign.
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.
Has been addressed.
def _unitary_(self) -> np.ndarray: | ||
if self._is_parameterized_(): | ||
return None | ||
return np.diag( |
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.
Does https://github.com/quantumlib/Cirq/pull/2591/files#r355092235 work for tuples?
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 did not work for tuples. I got a the following error. TypeError: can't multiply sequence by non-int of type 'complex'
.
return NotImplemented | ||
for index, angle in enumerate(self._diag_angles_radians): | ||
subspace_index = args.subspace_index(big_endian_bits_int=index) | ||
args.target_tensor[subspace_index] *= np.exp(1j * angle) |
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.
What happens with https://github.com/quantumlib/Cirq/pull/2591/files#r355092574?
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.
args.target_tensor *= np.exp(1j * self._diag_angles_radians.reshape((2,2)))
did not work since self._diag_angles_radians
was not a numpy.array
. Similar to above, the np.exp(1j * self_diag_angles_radians)
didn't work with tuples.
return NotImplemented | ||
angles = [] | ||
for angle in self._diag_angles_radians: | ||
mulAngle = protocols.mul(angle, exponent, NotImplemented) |
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.
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 thought that the code I added returned NotImplemented
from the __pow__
function. If I should address the comment in another way, 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.
ping @cduck
sqrt_diagonal_gate = diagonal_gate**.5 | ||
|
||
expected_angles = [prime / 2 for prime in diagonal_angles] | ||
np.testing.assert_allclose(expected_angles, |
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.
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.
Has been addressed.
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.
Merging as "pretty decent" reasons were given for not fixing some of @cduck's suggestions.
Automerge cancelled: "automerge" label was never added. |
Automerge cancelled: "automerge" label was never added. |
* Add TwoQubitDiagonalGate format Respond to reviews revetr revert again ignore global phase lint Fixed edits for the Add TwoQubitDiagonalGate. Change the docstring for the two_qubit_diagonal_gate.py * Pull request review edits. * GitHub merge fail fixes. * Update two_qubit_diagonal_gate.py Co-authored-by: Victory Omole <vtomole2@gmail.com>
Part of #2587