Remove axes from ActOnArgs, pass qubits explicitly to act_on

[daxfohl]

The current implementation of ActOnArgs requires two steps to "act on" an operation. First you have to manually set the axes, then you call the act_on protocol. This is confusing and creates opportunities for bugs to slip in when users forget to set the axes, and it shouldn't be necessary because the Operation that's being passed to act_on contains all the necessary information.

This PR fixes that problem by requiring qubits to be passed explicitly to the act_on protocol, either by passing as the action parameter an Operation that contains the qubits, or by explicitly passing the qubits parameter if the action is a gate or similar. Note there's a check, you cannot pass both an Operation and assign the qubits parameter; it has to be one or the other.

It touches a lot of files and lines, but it's mostly just adding the explicit parameter passing, and updating lots of unit tests to do so. The core changes are in act_on_args and act_on_protocol.

[95-martin-orion]

I'm committing to reviewing this PR this week.

[daxfohl]

Cool, one thing I've been thinking about since submitting this is, there's three different ways to do this. They're only stylistic differences, but we should choose the style we like before merging.

• Add an optional qubits parameter to act_on for use when something other than an Operation is provided.
  • (This PR).
  • Lowest-friction with all of the existing code.
  • Validity checks all occur at run time.
• Change the first parameter of act_on to Union[Operation, Tuple[Any, Sequence[Qid]]] (where Any should not be an Operation).
  • This is a little more explicit type-wise: you have to pass either an Operation or e.g. a (gate, *qubits) tuple. It's not possible to pass a gate without the qubits. (Though it's still possible to pass an operation with qubits, which can only be checked at run time).
  • But it feels a tad odd passing a tuple as an argument to a function.
• Split into two functions: act_on(Operation, ActOnArgs) and act_on_with_qubits(Any, Sequence[Qid], ActOnArgs]), again where Any should not be an operation.
  • May be a slightly bigger change because of changing the function names. A tad worried about the potential for gotchas with this approach, but can't think of anything specific.

I'm not hugely in favor of any one versus any of the others. I could write up an RFC but I don't think there's much to add beyond the above.

[daxfohl]

Actually I lied. I think I'm slightly in favor of the third option if it comes down to it.

And given this would be a breaking change for anyone currently using ActOnArgs.axes (though my guess is nobody is doing this), I'm willing to write up an RFC on this if you think it would be useful.

[95-martin-orion]

I also like the reduced ambiguity of the third option listed. That said, you're right about this being a breaking change - and by extension, unless we're really confident that there are no external users for this, we'll need to go through the appropriate deprecation steps (likely involving cirq.deprecated_parameter).

Let's bring this up in the Cirq sync to check - I've marked this with triage/discuss to make sure we cover it.

[95-martin-orion]

An RFC shouldn't be necessary, unless you think it would make presenting this decision clearer.

[daxfohl]

SGTM, though my availability for cynq is still limited.

If we do end up deciding to go with the full safe deprecation path, this is definitely going to add some complexity. Just marking the axes field as deprecated is insufficient because this code no longer uses axes, so it would be silently ignored: a worse experience than failing to compile. Maybe a check if-qubits-is-supplied-then-use-them-else-fallback-to-axes might work. (That would still cause a behavior change for anyone who is currently explicitly providing axes and then calling act_on with an operation that uses different qubits--current behavior uses the axes; after change it would use op's qubits. But can't think of a way around that yet. That said, I can't think of any valid reason anyone would be doing this).

[daxfohl]

Changed to the third option. I like it better.

[daxfohl]

Added a deprecation path as mentioned in the above comment. ActOnArgs.axes is back, but deprecated, and protocols.act_on will use it if a non-Operation is passed in. A test case to that effect is added too. Deprecation is marked to v0.13. All in the previous commit.

[daxfohl]

deprecated and deprecated_parameter is added everywhere they're needed. Unit tests added to check for backwards compatibility.

This should be ready to go.

[95-martin-orion]

From @cduck at the Cirq sync: prefer not having a separate dunder method for the new protocol.

[95-martin-orion]

From @cduck at the Cirq sync: prefer not having a separate dunder method for the new protocol.

i.e., if we have a separate act_on_with_qubits method, it should still use the _act_on_ magic method under the hood. Note that this is open for discussion - it was a preference raised in the meeting, not a binding decision.

[daxfohl]

Pretty sure that could be done with a global find/replace, so no problem accommodating that request if we decide to go that way.

[daxfohl]

If we do, I kind of feel like we should also go back to the original way of having the optional qubits parameter in the original act_on protocol, rather than creating a separate act_on_qubits protocol. My Java/C# self prefers the explicitness of having distinct protocols, but perhaps it's more pythonic to have a single function that can be used for both cases. ...In fact as I reflect further on it, I'm pretty sure it is.

Shall I change it back to a single protocol then?

[95-martin-orion]

Shall I change it back to a single protocol then?

I think that's reasonable. The only counterexample to this I can find in Cirq is decompose_once[_with_qubits], which are two separate protocol methods that both use _decompose_ under the hood...but they require passing *args and **kwargs around, which isn't great from a type-checking perspective.

[daxfohl]

gtg now