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

dabacon · 2022-04-08T19:13:17Z

This makes the API documentation for our gate constants better. Not sure why it trips the type notation.

tanujkhattar · 2022-04-08T19:29:51Z

cirq-core/cirq/ops/raw_types.py

-    def __call__(self, *args, **kwargs):
-        return self.on(*args, **kwargs)
+    def __call__(self, *qubits: Qid, **kwargs):
+        return self.on(*qubits)


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

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.

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 ?)

tanujkhattar · 2022-04-08T20:51:19Z

cirq-core/cirq/ops/raw_types.py

@@ -211,11 +211,13 @@ def validate_args(self, qubits: Sequence['cirq.Qid']) -> None:
        """
        _validate_qid_shape(self, qubits)

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


This is a breaking change because all user defined classes which override the on method as def on(self, *qubits: Qid) will now not satisfy LSP and would need to be updated?

yeah agree this is less than happy, let me see if I can work around the other issue (we currently call on in call with the wrong signature and get away with it)

Yeah, I think it wasn't an issue earlier because mypy was not type-checking the method at all, due to no type annotations. But now that's not the case and hence it raises an error.

We can also decide to make __call__ a more restrictive type on the base class, so if user overrides on, they must also override __call__ -- but that would also be a breaking change.

Maybe @maffoo knows some mypy magic to get around the situation without having to deal with breaking changes.

Maybe @maffoo knows some mypy magic to get around the situation without having to deal with breaking changes.

I got nothin', sorry :-)

dabacon · 2022-04-08T22:37:53Z

@tanujkhattar PTAL

I think this is the minimal change. It leaves the signatures the same, and to get around the CXPow gate it adds an explicit override for the __call__, which is not horrible. There is also a change necessary in GateOperation repr for an assert, but I think that assert is way too defensive.

tanujkhattar · 2022-04-09T01:39:20Z

So, what we have now is also a breaking change because every user defined class which was dependant on Gate.__call__ to forward the **kwargs to it's on method (like CXPowGate) will now break. We would ideally need to go through a deprecation cycle to make this change to avoid breaking user code directly.

dabacon · 2022-04-11T18:37:26Z

So, what we have now is also a breaking change because every user defined class which was dependant on Gate.__call__ to forward the **kwargs to it's on method (like CXPowGate) will now break. We would ideally need to go through a deprecation cycle to make this change to avoid breaking user code directly.

Hm, not sure I agree about that. This doesn't break any public interface, and if someone chose to follow the pattern of CXPowGate, they would be doing so in a manner that isn't really explicitly supported by any documented interface on Cirq's end.

tanujkhattar

Okay, I don't have very strong opinions here.

cc @MichaelBroughton and @95-martin-orion Do you have strong opinions on silently changing the behavior of a public interface and whether this should be a breaking change or not? We can merge if other maintainers don't have concerns.

95-martin-orion

LGTM, all interfaces remain the same (modulo better type annotation) and internal refactors have the same end behavior.

MichaelBroughton · 2022-04-11T20:39:21Z

I'm not a huge fan of this change solely in the name of getting better type hints (which has no functional impact by itself). There's also no super clear guidance on type hinting for *args and **kwargs. If it's just for docs, what if we just added type hints and then immediately washed them out with ignore comments ? That would get us the working docs (hopefully) and keep existing behavior.

dabacon · 2022-04-11T22:33:26Z

I'm not a huge fan of this change solely in the name of getting better type hints (which has no functional impact by itself). There's also no super clear guidance on type hinting for *args and **kwargs. If it's just for docs, what if we just added type hints and then immediately washed them out with ignore comments ? That would get us the working docs (hopefully) and keep existing behavior.

I'm not sure I follow. Why would we ignore the type hints?

MichaelBroughton · 2022-04-11T23:12:37Z

So that we could fix the API docs issue without having to introduce any code changes until we can come up with a fix everyone was on board with. Maybe I'm missing something here, is this a docs motivated PR or do we want the code fix itself in this PR too ?

dabacon · 2022-04-16T14:44:37Z

So that we could fix the API docs issue without having to introduce any code changes until we can come up with a fix everyone was on board with. Maybe I'm missing something here, is this a docs motivated PR or do we want the code fix itself in this PR too ?

The primary goal is to make sure all the gate constants have a reasonable doc string which shows they can be called on qubits. The type gets dropped by our beloved tensorflow doc overlords :)

This doesn't end up changing any interface and really just changes where passing in kwargs fails for on and __call__. Prior to this it will fail with a python error about on not supporting this if you call it on on or __call__. After this change it will fail by yelling about on when you call on and __call__ when you call _call__. So for tracing problems, it is actually slightly better. For overloading these methods if you want to accept kwargs, you will have to overload both now (this is what is done in the CX case). This reads somewhat more self contained, but is slightly worse from a code reuse perspective.

Really there was a bug as they should have been consistent with each other but fixing that is a breaking change.

dabacon · 2022-04-27T17:33:28Z

@MichaelBroughton let me know if you are OK with this, if not, also great and I will close this :)

This makes the API documentation for our gate constants better. Not sure why it trips the type notation. ![image](https://user-images.githubusercontent.com/87974/162509043-87824a51-0a46-4346-a9ec-779202d56a26.png) Fixes quantumlib#5232

dabacon added 2 commits April 8, 2022 12:06

Change signature of __call__ on Gate

67b6098

nit

baaa86c

dabacon requested review from a team, vtomole and cduck as code owners April 8, 2022 19:13

dabacon requested a review from maffoo April 8, 2022 19:13

CirqBot added the Size: XS <10 lines changed label Apr 8, 2022

tanujkhattar reviewed Apr 8, 2022

View reviewed changes

tanujkhattar self-assigned this Apr 8, 2022

dabacon added 2 commits April 8, 2022 12:58

pipe correctly

bc24296

more piping

bb789e7

CirqBot added the size: S 10< lines changed <50 label Apr 8, 2022

tanujkhattar requested changes Apr 8, 2022

View reviewed changes

dabacon added 3 commits April 8, 2022 14:43

revert

a4a0ae9

Merge branch 'master' into docargs

e761b93

Better fix

35da5cb

Merge branch 'master' into docargs

3ca09f2

tanujkhattar approved these changes Apr 11, 2022

View reviewed changes

95-martin-orion approved these changes Apr 11, 2022

View reviewed changes

dabacon requested a review from MichaelBroughton April 19, 2022 18:10

MichaelBroughton approved these changes Apr 27, 2022

View reviewed changes

MichaelBroughton added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Apr 27, 2022

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Apr 27, 2022

Merge branch 'master' into docargs

319841a

CirqBot merged commit cdabadf into quantumlib:master Apr 27, 2022

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 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dabacon commented Apr 8, 2022 •

edited by tanujkhattar

tanujkhattar Apr 8, 2022 •

edited

tanujkhattar Apr 8, 2022 •

edited

dabacon Apr 8, 2022

tanujkhattar Apr 8, 2022

dabacon Apr 8, 2022

tanujkhattar Apr 8, 2022

maffoo Apr 8, 2022

dabacon commented Apr 8, 2022

tanujkhattar commented Apr 9, 2022

dabacon commented Apr 11, 2022

tanujkhattar left a comment

95-martin-orion left a comment

MichaelBroughton commented Apr 11, 2022

dabacon commented Apr 11, 2022

MichaelBroughton commented Apr 11, 2022

dabacon commented Apr 16, 2022

dabacon commented Apr 27, 2022

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

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

Conversation

dabacon commented Apr 8, 2022 • edited by tanujkhattar

tanujkhattar Apr 8, 2022 • edited

Choose a reason for hiding this comment

tanujkhattar Apr 8, 2022 • edited

Choose a reason for hiding this comment

dabacon Apr 8, 2022

Choose a reason for hiding this comment

tanujkhattar Apr 8, 2022

Choose a reason for hiding this comment

dabacon Apr 8, 2022

Choose a reason for hiding this comment

tanujkhattar Apr 8, 2022

Choose a reason for hiding this comment

maffoo Apr 8, 2022

Choose a reason for hiding this comment

dabacon commented Apr 8, 2022

tanujkhattar commented Apr 9, 2022

dabacon commented Apr 11, 2022

tanujkhattar left a comment

Choose a reason for hiding this comment

95-martin-orion left a comment

Choose a reason for hiding this comment

MichaelBroughton commented Apr 11, 2022

dabacon commented Apr 11, 2022

MichaelBroughton commented Apr 11, 2022

dabacon commented Apr 16, 2022

dabacon commented Apr 27, 2022

dabacon commented Apr 8, 2022 •

edited by tanujkhattar

tanujkhattar Apr 8, 2022 •

edited

tanujkhattar Apr 8, 2022 •

edited