CNOT doesn't decompose to CNOT #5712
Conversation
|
[sc-63619] |
There was a problem hiding this comment.
Thanks @lillian542, this looks good to me! Good catch :)
I just had a tiny suggestion, would ask to remove a print line, and wanted to confirm I understand the background correctly with a question :)
edit: Actually, this change might warrant a breaking change entry in the changelog, given the "failing" tape expansion tests and that CNOTs are omnipresent in standard circuits.
| *qml.CY(wires=[1, 4]).decomposition(), | ||
| *qml.CZ(wires=[1, 0]).decomposition(), | ||
| qml.PauliX(wires=1), | ||
| ] | ||
| assert len(tape) == 9 | ||
| expanded = tape.expand(stop_at=lambda obj: not isinstance(obj, Controlled)) | ||
| print(expanded.circuit) |
There was a problem hiding this comment.
| print(expanded.circuit) |
| assert len(tape.operations) == 10 | ||
| assert all(o.name in {"CNOT", "CRX", "Toffoli"} for o in tape.operations) | ||
| assert len(tape.operations) == 12 | ||
| assert all(o.name in {"ControlledPhaseShift", "CRX", "Toffoli"} for o in tape.operations) |
There was a problem hiding this comment.
Just to make sure I understand: The requirement for a decomposition is not to imply any ordering of "complex gates" to "simpler gates", but just should allow us to keep decomposing until external stopping criteria (like those of a device) trigger? :)
| @@ -56,6 +56,9 @@ def _check_decomposition(op): | |||
| expand = op.expand() | |||
|
|
|||
| assert isinstance(decomp, list), "decomposition must be a list" | |||
| assert op.__class__ not in [ | |||
| decomp_op.__class__ for decomp_op in decomp | |||
| ], "an operator should not be included in its own decomposition" | |||
There was a problem hiding this comment.
I actually would have expected some other operation popping up to have this bug here :D
| assert op.__class__ not in [ | ||
| decomp_op.__class__ for decomp_op in decomp | ||
| ], "an operator should not be included in its own decomposition" |
There was a problem hiding this comment.
Could do
| assert op.__class__ not in [ | |
| decomp_op.__class__ for decomp_op in decomp | |
| ], "an operator should not be included in its own decomposition" | |
| assert op.__class__ not in { | |
| decomp_op.__class__ for decomp_op in decomp | |
| }, "an operator should not be included in its own decomposition" |
Agreed, I want to bring this up in stand up tomorrow and make sure it's something we actually want (I think it is, just want to confirm). Hopefully most people are decomposing in ways that include But it could be between this and |
|
So just to summarise our discussion in the standup, here we are effectively reverting to behaviour in Q1? |
Yes - may or may not get back to this until next iteration, but the plan is to see what it used to do before we made |
|
Is this to be reopened later on? |
|
Oops! I forgot to change the target branch, and this happens when the target branch is deleted. It also happened for one of Matthew's PRs last time I was managing the release, and I think he had to re-create it, we weren't able to re-open it. But we can open a new PR from the same branch. |
|
Now re-opened as #6039 |
Context:
Up to and including Pennylane 0.34, the CNOT gate raised a
DecompositionUndefinedErrorin itsdecompositionmethod. We added some smart handling of decompositions for controlled operators that accidentally started decomposing CNOT into itself.Description of the Change:
Make it so
_decompose_pauli_x_based_no_control_valuesno longer provides a decomposition forCNOT(it currently just sees a controlled op with a base ofPauliX, and decomposes toCNOT). The decomposition functions then follow another pathway and return another decomposition for CNOT.Benefits:
This fringe but odd behaviour doesn't happen anymore
Possible Drawbacks:
I tested it by adding a check to
assert_valid, maybe this problem is too much of an edge case to be inassert_valid. But with the controlled ops finding their own decompositions instead of having them hardcoded, I guess it could happen again 🤷♀️Related GitHub Issues:
#5711