Add a test for an uncovered case involving add_op_to_opstring#1061
Add a test for an uncovered case involving add_op_to_opstring#1061mhucka wants to merge 2 commits into
add_op_to_opstring#1061Conversation
Added a new test case `test_add_op_to_opstring_controlled_gate_error` to `qsimcirq_tests/qsimcirq_test.py` to verify that `add_op_to_opstring` correctly raises a `ValueError` when a controlled operation is passed.
There was a problem hiding this comment.
Code Review
This pull request adds a new test case, test_add_op_to_opstring_controlled_gate_error, to verify that add_op_to_opstring correctly raises a ValueError when encountering unsupported gate types. Feedback was provided indicating that the current test implementation using controlled_by would trigger a qubit count validation error before reaching the intended gate kind check. Additionally, the reviewer noted that the gate kind would be identified as kX rather than kCX, and suggested using cirq.CX to correctly trigger and validate the expected error message.
|
|
||
| def test_add_op_to_opstring_controlled_gate_error(): | ||
| q0, q1 = cirq.LineQubit.range(2) | ||
| op = cirq.X(q0).controlled_by(q1) |
There was a problem hiding this comment.
The test uses cirq.X(q0).controlled_by(q1), which is a 2-qubit operation. In add_op_to_opstring, this will trigger the qubit count validation (len(qsim_op.qubits) != 1) at line 254 of qsim_circuit.py before it can reach the gate kind check. Furthermore, _cirq_gate_kind for a ControlledGate returns the kind of its sub-gate, so for cirq.X.controlled_by(q1), the kind would be kX, not kCX. To match the expected error message and correctly trigger the gate kind validation, use cirq.CX(q1, q0) instead.
| op = cirq.X(q0).controlled_by(q1) | |
| op = cirq.CX(q1, q0) |
Added a new test case
test_add_op_to_opstring_controlled_gate_errortoqsimcirq_tests/qsimcirq_test.pyto verify thatadd_op_to_opstringcorrectly raises aValueErrorwhen a controlled operation is passed.