Skip to content
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 assert_specifies_has_unitary_if_unitary to consistency tests #1759

Merged
merged 22 commits into from
Apr 28, 2020

Conversation

Strilanc
Copy link
Contributor

@Strilanc Strilanc commented Jun 27, 2019

No description provided.

@Strilanc Strilanc requested a review from dabacon June 27, 2019 20:15
@googlebot googlebot added the cla: yes Makes googlebot stop complaining. label Jun 27, 2019
@Strilanc Strilanc requested a review from cduck July 17, 2019 02:19
@cduck
Copy link
Collaborator

cduck commented Jul 17, 2019

Is this intended to change the protocol to require _has_unitary_ or to just require that we always use _has_unitary_ within Cirq?

@Strilanc
Copy link
Contributor Author

It's intended to ensure we always use it within cirq

cirq/ops/raw_types.py Show resolved Hide resolved
cirq/testing/__init__.py Show resolved Hide resolved
cirq/testing/consistent_protocols.py Show resolved Hide resolved
@@ -33,6 +33,9 @@ def __init__(self,
self.phase_exponent = cirq.canonicalize_half_turns(phase_exponent)
self.exponent = exponent

def _has_unitary_(self):
return not cirq.is_parameterized(self)

def _unitary_(self) -> Union[np.ndarray, NotImplementedType]:
if cirq.is_parameterized(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Inconsistent with other places.

@@ -33,6 +33,9 @@ def __init__(self,
self.phase_exponent = cirq.canonicalize_half_turns(phase_exponent)
self.exponent = exponent

def _has_unitary_(self):
return not cirq.is_parameterized(self)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Inconsistent with other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How so?

@@ -66,6 +66,9 @@ def _is_parameterized_(self):
return cirq.is_parameterized(self.theta) or cirq.is_parameterized(
self.phi)

def _has_unitary_(self):
return not self._is_parameterized_()

def _unitary_(self) -> Optional[np.ndarray]:
if cirq.is_parameterized(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Inconsistent with other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what way?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Often self._is_parameterized_() is used instead.

@cduck
Copy link
Collaborator

cduck commented Jul 22, 2019

Ping @Strilanc.

# Conflicts:
#	cirq/ops/common_gates.py
#	cirq/ops/matrix_gates.py
#	cirq/ops/raw_types.py
#	cirq/testing/consistent_protocols.py
@Strilanc
Copy link
Contributor Author

Ping @cduck

@Strilanc Strilanc added the Ready for Re-Review For when reviewers take their time. label Nov 19, 2019
Copy link
Collaborator

@cduck cduck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just one qudit compatibility issue.

@@ -66,6 +66,9 @@ def _is_parameterized_(self):
return cirq.is_parameterized(self.theta) or cirq.is_parameterized(
self.phi)

def _has_unitary_(self):
return not self._is_parameterized_()

def _unitary_(self) -> Optional[np.ndarray]:
if cirq.is_parameterized(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Often self._is_parameterized_() is used instead.

@@ -449,6 +449,14 @@ def _decompose_(self, qubits):
return protocols.inverse(
protocols.decompose_once_with_qubits(self._original, qubits))

def _has_unitary_(self):
from cirq import protocols, devices
qubits = devices.LineQubit.range(self._num_qubits_())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be LineQid.for_gate(self).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@Strilanc
Copy link
Contributor Author

Often self.is_parameterized() is used instead.

Done

@Strilanc Strilanc added automerge Tells CirqBot to sync and merge this PR. (If it's running.) and removed Ready for Re-Review For when reviewers take their time. labels Nov 20, 2019
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Nov 20, 2019
@CirqBot
Copy link
Collaborator

CirqBot commented Nov 20, 2019

Automerge cancelled: A status check is failing.

@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Nov 20, 2019
@cduck
Copy link
Collaborator

cduck commented Nov 20, 2019

Did you intentionally remove IdentityOperation.gate or is that a merge issue?

@Strilanc
Copy link
Contributor Author

It was a merge issue

@CirqBot CirqBot removed the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Nov 21, 2019
@cduck cduck added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Nov 27, 2019
@CirqBot
Copy link
Collaborator

CirqBot commented Nov 27, 2019

Automerge cancelled: There are merge conflicts.

@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Nov 27, 2019
@cduck
Copy link
Collaborator

cduck commented Nov 27, 2019

Ping @Strilanc.

@Strilanc Strilanc added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Dec 17, 2019
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Dec 17, 2019
@CirqBot
Copy link
Collaborator

CirqBot commented Dec 17, 2019

Automerge cancelled: A status check is failing.

@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Dec 17, 2019
@Strilanc Strilanc added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Apr 28, 2020
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Apr 28, 2020
@CirqBot
Copy link
Collaborator

CirqBot commented Apr 28, 2020

Automerge cancelled: A status check is failing.

@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Apr 28, 2020
@Strilanc Strilanc added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Apr 28, 2020
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Apr 28, 2020
@CirqBot CirqBot merged commit a3af71f into master Apr 28, 2020
@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Apr 28, 2020
@CirqBot CirqBot deleted the density_sanityG branch April 28, 2020 22:27
@CirqBot CirqBot removed the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants