Skip to content

Fix call signature on Gate to show that it takes Qids. #5235

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Apr 27, 2022
Merged

Fix call signature on Gate to show that it takes Qids. #5235

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
67b6098
Change signature of __call__ on Gate
dabacon Apr 8, 2022
baaa86c
nit
dabacon Apr 8, 2022
bc24296
pipe correctly
dabacon Apr 8, 2022
bb789e7
more piping
dabacon Apr 8, 2022
a4a0ae9
revert
dabacon Apr 8, 2022
e761b93
Merge branch 'master' into docargs
dabacon Apr 8, 2022
35da5cb
Better fix
dabacon Apr 8, 2022
3ca09f2
Merge branch 'master' into docargs
dabacon Apr 8, 2022
319841a
Merge branch 'master' into docargs
CirqBot Apr 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cirq-core/cirq/contrib/quil_import/quil.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

from typing import Callable, Dict, Union
from typing import Callable, cast, Dict, Union

import numpy as np
from pyquil.parser import parse
Expand Down Expand Up @@ -258,7 +258,7 @@ def circuit_from_quil(quil: str) -> Circuit:
raise UndefinedQuilGate(f"Quil gate {quil_gate_name} not supported in Cirq.")
cirq_gate_fn = defined_gates[quil_gate_name]
if quil_gate_params:
circuit += cirq_gate_fn(*quil_gate_params)(*line_qubits)
circuit += cast(Callable[..., Gate], cirq_gate_fn)(*quil_gate_params)(*line_qubits)
else:
circuit += cirq_gate_fn(*line_qubits)

Expand Down
4 changes: 2 additions & 2 deletions cirq-core/cirq/ops/raw_types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,8 +319,8 @@ def __pow__(self, power):

return NotImplemented

def __call__(self, *args, **kwargs):
return self.on(*args, **kwargs)
def __call__(self, *qubits: Qid, **kwargs):
return self.on(*qubits)
Copy link
Collaborator

@tanujkhattar tanujkhattar Apr 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You also need to pass the **kwargs to self.on() call, so that cirq.CNOT(control=a, target=b) works ?

Copy link
Collaborator

@tanujkhattar tanujkhattar Apr 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The cirq.Gate class defines the on method as:

def on(self, *qubits: Qid) -> 'Operation':
    ...

whereas it's derived class, CXPowGate, overrides the on method as

def on(self, *args: 'cirq.Qid', **kwargs: 'cirq.Qid') -> raw_types.Operation:
    ...

Technically, this does satisfy the liskov substitution principle because the derived class is implementing a more generic method (that accepts both *args and **kwargs) compared to the base class (that can accept only *args).
I think your current solution, to not specify type of **kwargs in __call__ and silently forwarding it to self.on(), should work.

One other way could be to enforce that the cirq.Gate's __call__ method will only accept *qubits and whenever a derived class, like CXPowGate, overrides the on method to be more generic, it should also override the __call__ method? But that would be a huge breaking change for all existing implementations, like CXPowGate, and something we probably don't want to do at this point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes sorry thought I had fixed this. This is what happens when check/all doesn't run tests in parallel so I just push it out without running the tests (I can haz review for #5006 ?)


def with_probability(self, probability: 'cirq.TParamVal') -> 'cirq.Gate':

Expand Down