Rename CX class to XCX to reduce confusion. #140
Conversation
| @@ -7,7 +7,7 @@ | |||
| from .simplify import id_simp, tcount | |||
| from .rules import apply_rule, pivot, match_spider_parallel, spider | |||
| from .circuit import Circuit | |||
| from .circuit.gates import Gate, ParityPhase, CNOT, HAD, ZPhase, XPhase, CZ, CX, SWAP, InitAncilla | |||
| from .circuit.gates import Gate, ParityPhase, CNOT, HAD, ZPhase, XPhase, CZ, XCX, SWAP, InitAncilla | |||
There was a problem hiding this comment.
(The XCX gate is not actually used in this file, nor are many of the other imported gates, but as the comment at the top says it is experimental, presumably it is intended to be used at some point.)
pyzx/circuit/gates.py
Outdated
| @@ -966,7 +967,7 @@ def to_graph(self, g, q_mapper, c_mapper): | |||
| "CNOT": CNOT, | |||
| "CZ": CZ, | |||
| "ParityPhase": ParityPhase, | |||
| "CX": CX, | |||
| "CX": XCX, | |||
There was a problem hiding this comment.
Did you miss changing the name here?
There was a problem hiding this comment.
The intention in this PR was to leave external behaviour unchanged. IIUC, the string here affects the API in terms of the input to add_gate, prepend_gate, etc., so that users would have to change their calling behaviour if this was changed. That is, it would render existing code using pyzx backwards-incompatible.
For a user-facing change like that, it think it should be done in a separate PR, so it can be rolled back if necessary. The API should probably also be made to accept the previous input value, maybe with a warning message. I can make those changes in a follow-up PR. I don't know how you've managed breaking API changes in the past, but typically one would make an announcement to a user mailing list, or the change would be made at a new version number with a changelog that notes the backwards-incompatibility.
For now, changing the name of the gate just internally makes the code less confusing (at least for me!) to read.
btw, I noticed that qasm_gate_table has two entries for CNOT, lowercase "cx" and uppercase "CX", which is not done for any other gate. What's the reason for this?
There was a problem hiding this comment.
You're right that changing the name there would change the user-facing API, but that is also the case for changing the name CX to XCX, as the Gate classes are also user-facing. The last time we had significant backwards-compatibility breaking changes I bumped the significant bit of the version number, so we could do that here as well. In any case it has been overdue for a new release, and quite a few things have changed, so that wouldn't be a problem.
There was a problem hiding this comment.
With regards to qasm_gate_table, I see that I also have special behaviour for this in qasmparser.py, so it was apparently necessary for something at some point. To be honest, I don't even know whether QASM is canonically case-insensitive, and whether it would be wrong to just case everything to lowercase.
There was a problem hiding this comment.
Okay, I've changed the name in the gate_types as well.
According to the qasm paper, section 2, "The language is case sensitive."
XCX represents an X-controlled-X gate, whereas CX is typically a synonym for CNOT, i.e., a Z-controlled-X gate. Fixes zxcalc#139.
dlyongemallo
force-pushed
the
rename_xcx
branch
from
3552175 to
7dbf260
Compare
XCX represents an X-controlled-X gate, whereas CX is typically a synonym for CNOT, i.e., a Z-controlled-X gate.