Port CRX/Y/Z gates to Rust

Cryoris commented 6 days ago

Summary

Port the controlled Pauli rotations to Rust. Checks off parts of #12566.

qiskit-bot commented 6 days ago

One or more of the following people are relevant to this code:

@Cryoris
@Qiskit/terra-core
@ajavadia
@kevinhartman
@mtreinish

coveralls commented 6 days ago

Pull Request Test Coverage Report for Build 9650794209

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

106 of 115 (92.17%) changed or added relevant lines in 5 files are covered.
5 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.02%) to 89.78%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/operations.rs	72	81	88.89%
<!--	Total:	106	115	92.17%	-->

Files with Coverage Reduction	New Missed Lines	%
crates/circuit/src/operations.rs	1	75.93%
crates/qasm2/src/lex.rs	4	91.86%
<!--	Total:	5	-->

Totals
Change from base Build 9648699798:	0.02%
Covered Lines:	63847
Relevant Lines:	71115

💛 - Coveralls

coveralls commented 6 days ago

Pull Request Test Coverage Report for Build 9659942591

Details

106 of 115 (92.17%) changed or added relevant lines in 5 files are covered.
24 unchanged lines in 4 files lost coverage.
Overall coverage decreased (-0.02%) to 89.735%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/operations.rs	72	81	88.89%
<!--	Total:	106	115	92.17%	-->

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/expr.rs	1	94.02%
crates/circuit/src/operations.rs	1	73.44%
crates/qasm2/src/lex.rs	4	92.62%
crates/qasm2/src/parse.rs	18	96.69%
<!--	Total:	24	-->

Totals
Change from base Build 9650973845:	-0.02%
Covered Lines:	63862
Relevant Lines:	71167

💛 - Coveralls

coveralls commented 6 days ago

Pull Request Test Coverage Report for Build 9665498437

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

112 of 121 (92.56%) changed or added relevant lines in 6 files are covered.
17 unchanged lines in 4 files lost coverage.
Overall coverage decreased (-0.006%) to 89.748%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/operations.rs	72	81	88.89%
<!--	Total:	112	121	92.56%	-->

Files with Coverage Reduction	New Missed Lines	%
crates/circuit/src/operations.rs	1	73.44%
crates/qasm2/src/lex.rs	1	93.64%
qiskit/circuit/quantumcircuit.py	3	95.52%
crates/qasm2/src/parse.rs	12	97.15%
<!--	Total:	17	-->

Totals
Change from base Build 9650973845:	-0.006%
Covered Lines:	63876
Relevant Lines:	71173

💛 - Coveralls

Cryoris commented 4 days ago

Picking up the ctrl_state edge case @ElePT mentioned above, there is a potential sharp bit we're introducing with the change to Rust (this happens independently of the None vs 1 "1" discussion): changing the ctrl_state of a mutable gate, like CRX, once the gate is in the circuit does not change the matrix anymore, but did before.

For example

from qiskit.quantum_info import Operator
from qiskit.circuit import QuantumCircuit

qc = QuantumCircuit(2)
qc.crx(2, 0, 1)
print(Operator(qc))  # matrix with ctrl_state 1

# not very legal but not explicitly illegal?
qc.data[0].operation.ctrl_state = 0
# w/o Rust gates this prints the correct matrix with ctrl_state 0,
# with Rust gates it still shows the matrix for ctrl_state 1
print(Operator(qc))

This may be something we need to fix globally.

coveralls commented 4 days ago

Pull Request Test Coverage Report for Build 9692730727

Details

112 of 121 (92.56%) changed or added relevant lines in 6 files are covered.
10 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.001%) to 89.783%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/operations.rs	72	81	88.89%
<!--	Total:	112	121	92.56%	-->

Files with Coverage Reduction	New Missed Lines	%
crates/circuit/src/operations.rs	1	73.95%
qiskit/circuit/quantumcircuit.py	3	95.51%
crates/qasm2/src/lex.rs	6	92.37%
<!--	Total:	10	-->

Totals
Change from base Build 9683280067:	-0.001%
Covered Lines:	63922
Relevant Lines:	71196

💛 - Coveralls

ElePT commented 4 days ago

Picking up the ctrl_state edge case @ElePT mentioned above, there is a potential sharp bit we're introducing with the change to Rust (this happens independently of the None vs 1 "1" discussion): changing the ctrl_state of a mutable gate, like CRX, once the gate is in the circuit does not change the matrix anymore, but did before.

For example
from qiskit.quantum_info import Operator
from qiskit.circuit import QuantumCircuit

qc = QuantumCircuit(2)
qc.crx(2, 0, 1)
print(Operator(qc))  # matrix with ctrl_state 1

# not very legal but not explicitly illegal?
qc.data[0].operation.ctrl_state = 0
# w/o Rust gates this prints the correct matrix with ctrl_state 0,
# with Rust gates it still shows the matrix for ctrl_state 1
print(Operator(qc)) 
This may be something we need to fix globally.

This sounds like a friend of:

gate.definition.global_phase = 3.14159
dag_node.op.params[0] = 3.14159

I don't think we'll be able to mutate any gate element post-Rust migration using that syntax (reno)

mtreinish commented 4 days ago

The other issue with ctrl_state that I realized we're missing right now is what I pointed out here: https://github.com/Qiskit/qiskit/pull/12659#discussion_r1654466050 the handling in CircuitInstruction's rust code for when you do CircuitInstruction(CRXGate(pi, ctrl_state=0), ...) doesn't understand that CRXGate with ctrl_state=0 is not the standard gate definition. We will need to update the logic in that function to check the value of ctrl_state for a non-singleton controlled gate.

ElePT commented 4 days ago

The other issue with ctrl_state that I realized we're missing right now is what I pointed out here: #12659 (comment) the handling in CircuitInstruction's rust code for when you do CircuitInstruction(CRXGate(pi, ctrl_state=0), ...) doesn't understand that CRXGate with ctrl_state=0 is not the standard gate definition. We will need to update the logic in that function to check the value of ctrl_state for a non-singleton controlled gate.

Yes, I am trying to fix it as part of #12659.

coveralls commented 3 days ago

Pull Request Test Coverage Report for Build 9710882058

Details

112 of 121 (92.56%) changed or added relevant lines in 6 files are covered.
10 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.03%) to 89.77%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/operations.rs	72	81	88.89%
<!--	Total:	112	121	92.56%	-->

Files with Coverage Reduction	New Missed Lines	%
crates/circuit/src/operations.rs	1	73.76%
qiskit/circuit/quantumcircuit.py	3	95.51%
crates/qasm2/src/lex.rs	6	92.11%
<!--	Total:	10	-->

Totals
Change from base Build 9703107599:	0.03%
Covered Lines:	63926
Relevant Lines:	71211

💛 - Coveralls

coveralls commented 3 days ago

Pull Request Test Coverage Report for Build 9714453170

Details

112 of 121 (92.56%) changed or added relevant lines in 6 files are covered.
6 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.006%) to 89.754%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/operations.rs	72	81	88.89%
<!--	Total:	112	121	92.56%	-->

Files with Coverage Reduction	New Missed Lines	%
crates/circuit/src/operations.rs	1	73.76%
crates/qasm2/src/lex.rs	2	92.88%
qiskit/circuit/quantumcircuit.py	3	95.51%
<!--	Total:	6	-->

Totals
Change from base Build 9714383578:	0.006%
Covered Lines:	63915
Relevant Lines:	71211

Qiskit / qiskit