Port CRX/Y/Z gates to Rust #12648

Cryoris · 2024-06-24T18:49:26Z

Summary

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

and prepare for U1/2/3 PR

qiskit-bot · 2024-06-24T18:49:31Z

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

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

coveralls · 2024-06-24T19:13:39Z

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%

Files with Coverage Reduction	New Missed Lines	%
crates/circuit/src/operations.rs	1	75.93%
crates/qasm2/src/lex.rs	4	91.86%

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

💛 - Coveralls

coveralls · 2024-06-25T09:52:27Z

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%

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%

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

💛 - Coveralls

mtreinish

Can you update the QuantumCircuit methods to directly append the rust gate too? You can look at cx or h for an example of how to do this.

coveralls · 2024-06-25T16:05:03Z

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%

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%

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

💛 - Coveralls

ElePT · 2024-06-26T08:10:47Z

qiskit/circuit/quantumcircuit.py

@@ -4776,6 +4776,12 @@ def crx(
        """
        from .library.standard_gates.rx import CRXGate

+        # if the control state is |1> use the fast Rust version of the gate
+        if ctrl_state is None or ctrl_state in ["1", 1]:


Hmm I get that you added the ["1", 1] cases because it's the state set by default when ctrl_state is None, but I wonder if we would be overseeing some edge case by adding this?? (If there is indeed no difference, I think we should do it in all controlled gate methods).

If ctrl_state is None it is directly set to 1 in the constructor of any controlled gate, e.g.

>>> CRXGate(theta=0.5, ctrl_state=None).ctrl_state 1

so I think there should be no functional difference 🙂

But I agree we should generally cover all |1> state definitions we can use the Rust implementation more generally.

Cryoris · 2024-06-27T07:43:08Z

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 · 2024-06-27T08:14:41Z

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%

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%

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

💛 - Coveralls

ElePT · 2024-06-27T09:19:15Z

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 · 2024-06-27T09:40:05Z

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.

ElePT · 2024-06-27T09:52:11Z

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.

ElePT

Thanks a lot, I added a path to handle custom control states in 9b47121, and I will also update the other circuit construction methods to add the ctrl_state in ["1", 1] case once this PR is merged.

coveralls · 2024-06-28T10:14:45Z

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%

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%

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

💛 - Coveralls

coveralls · 2024-06-28T14:55:49Z

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%

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%

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

💛 - Coveralls

* v0 of CR-Pauli gates * fix inevitable matrix typos * update multiply_param and prepare for U1/2/3 PR * fix num params/qubits * cct methods to append rust gates --------- Co-authored-by: Elena Peña Tapia <[email protected]>

Cryoris added 5 commits June 19, 2024 16:36

v0 of CR-Pauli gates

a193bcf

fix inevitable matrix typos

50869a0

Merge branch 'main' into rust/crpaulis

9c1d4c7

update multiply_param

c087960

and prepare for U1/2/3 PR

Merge branch 'main' into rust/crpaulis

25909b5

Cryoris added performance Rust This PR or issue is related to Rust code in the repository mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library labels Jun 24, 2024

Cryoris requested a review from a team as a code owner June 24, 2024 18:49

Cryoris added 2 commits June 25, 2024 11:05

Merge branch 'main' into rust/crpaulis

3730716

fix num params/qubits

c15b905

mtreinish reviewed Jun 25, 2024

View reviewed changes

cct methods to append rust gates

ed7ebe6

ElePT reviewed Jun 26, 2024

View reviewed changes

mtreinish added the Changelog: None Do not include in changelog label Jun 26, 2024

Merge branch 'main' into rust/crpaulis

6b8238c

Merge branch 'main' into rust/crpaulis

8ebebf1

ElePT approved these changes Jun 28, 2024

View reviewed changes

ElePT mentioned this pull request Jun 28, 2024

Add remaining standard gates to rust representation #12566

Closed

Merge branch 'main' into rust/crpaulis

ea51bbf

ElePT enabled auto-merge June 28, 2024 14:30

ElePT added this pull request to the merge queue Jun 28, 2024

Merged via the queue into Qiskit:main with commit 9b0a584 Jun 28, 2024
15 checks passed

Cryoris deleted the rust/crpaulis branch July 10, 2024 09:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port CRX/Y/Z gates to Rust #12648

Port CRX/Y/Z gates to Rust #12648

Cryoris commented Jun 24, 2024 •

edited

Loading

qiskit-bot commented Jun 24, 2024

coveralls commented Jun 24, 2024 •

edited

Loading

coveralls commented Jun 25, 2024 •

edited

Loading

mtreinish left a comment

coveralls commented Jun 25, 2024 •

edited

Loading

ElePT Jun 26, 2024

Cryoris Jun 27, 2024

Cryoris commented Jun 27, 2024

coveralls commented Jun 27, 2024 •

edited

Loading

ElePT commented Jun 27, 2024 •

edited

Loading

mtreinish commented Jun 27, 2024

ElePT commented Jun 27, 2024

ElePT left a comment

coveralls commented Jun 28, 2024 •

edited

Loading

coveralls commented Jun 28, 2024 •

edited

Loading

Port CRX/Y/Z gates to Rust #12648

Port CRX/Y/Z gates to Rust #12648

Conversation

Cryoris commented Jun 24, 2024 • edited Loading

Summary

qiskit-bot commented Jun 24, 2024

coveralls commented Jun 24, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9650794209

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

coveralls commented Jun 25, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9659942591

Details

💛 - Coveralls

mtreinish left a comment

Choose a reason for hiding this comment

coveralls commented Jun 25, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9665498437

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

ElePT Jun 26, 2024

Choose a reason for hiding this comment

Cryoris Jun 27, 2024

Choose a reason for hiding this comment

Cryoris commented Jun 27, 2024

coveralls commented Jun 27, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9692730727

Details

💛 - Coveralls

ElePT commented Jun 27, 2024 • edited Loading

mtreinish commented Jun 27, 2024

ElePT commented Jun 27, 2024

ElePT left a comment

Choose a reason for hiding this comment

coveralls commented Jun 28, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9710882058

Details

💛 - Coveralls

coveralls commented Jun 28, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9714453170

Details

💛 - Coveralls

Cryoris commented Jun 24, 2024 •

edited

Loading

coveralls commented Jun 24, 2024 •

edited

Loading

coveralls commented Jun 25, 2024 •

edited

Loading

coveralls commented Jun 25, 2024 •

edited

Loading

coveralls commented Jun 27, 2024 •

edited

Loading

ElePT commented Jun 27, 2024 •

edited

Loading

coveralls commented Jun 28, 2024 •

edited

Loading

coveralls commented Jun 28, 2024 •

edited

Loading