Implement RGate in Rust by jlapeyre · Pull Request #12662 · Qiskit/qiskit

jlapeyre · 2024-06-26T04:59:30Z

This completes all tasks of implementing RGate in Rust rather than Python.

This replaces

Implement some standard gates in rust #12507

#12507 has been modified and split into the present PR and #12651

qiskit-bot · 2024-06-26T04:59:35Z

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

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

coveralls · 2024-06-26T05:23:45Z

Pull Request Test Coverage Report for Build 9673800273

Details

17 of 47 (36.17%) changed or added relevant lines in 4 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.007%) to 89.713%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/operations.rs	3	33	9.09%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	2	92.37%

Totals
Change from base Build 9661630219:	-0.007%
Covered Lines:	63783
Relevant Lines:	71097

💛 - Coveralls

ElePT

Thanks for the PR! it looks pretty good, I just left a couple of comments.

ElePT · 2024-06-26T07:43:49Z

 from qiskit.circuit.library.standard_gates import get_standard_gate_name_mapping

-SKIP_LIST = {"rx", "ry", "ecr"}
+SKIP_LIST = {"rx", "ry", "ecr", "r"}


Why add the r gate to the SKIP_LIST? All others haven't been implemented yet, but r has an implementation.

Good question. I added and removed several times while debugging. I'll recheck.

Fixed in 7eef0f3 (#12662). It failed because the decomposition had an error.

ElePT · 2024-06-26T07:59:48Z

+                let theta_expr = match &params[0] {
+                    Param::Float(theta) => Param::Float(*theta),
+                    Param::ParameterExpression(theta) => {
+                        Param::ParameterExpression(theta.clone_ref(py))
+                    }
+                    Param::Obj(_) => unreachable!(),
+                };
+                let (phi_expr1, phi_expr2) = match &params[0] {
+                    Param::Float(phi) => (Param::Float(*phi - 1.0), Param::Float(-*phi + 1.0)),
+                    Param::ParameterExpression(phi) => {
+                        let phiexpr1 = phi
+                            .call_method1(py, intern!(py, "__add__"), ((-PI / 2.0),))
+                            .expect("Unexpected Qiskit python bug");
+                        let phiexpr2 = phiexpr1
+                            .call_method1(py, intern!(py, "__rmul__"), (-1.0,))
+                            .expect("Unexpected Qiskit python bug");
+                        (
+                            Param::ParameterExpression(phiexpr1),
+                            Param::ParameterExpression(phiexpr2),
+                        )
+                    }
+                    Param::Obj(_) => unreachable!(),
+                };
+                let defparams = smallvec![theta_expr, phi_expr1, phi_expr2];
+                Some(
+                    CircuitData::from_standard_gates(
+                        py,
+                        1,
+                        [(Self::UGate, defparams, smallvec![Qubit(0)])],
+                        FLOAT_ZERO,
+                    )
+                    .expect("Unexpected Qiskit python bug"),
+                )
+            }),


I think this could be more readable if we used helper functions like multiply_param, and maybe add one for adding a scalar to a parameter in the same fashion. But I am fine merging the implementation as-is and beautifying it later.

My first iteration did not fit with the helper function, nor one for add. But it's changed a lot since I last tried. I'll revisit it; readability and encapsulation is important.

I did something similar to your suggest in 49c8dcd (#12662). This factors out cloning a Param. This is only used once now, but can be used whenever a Param needs to be cloned. I find the code much more readable at the call point.

Regarding factoring out add_param: There are pros and cons to doing this. Disadvantages: The branch with -*phi + PI / 2.0 is more readable if it is not nested calls. In fact, this made it slightly easier for me to spot the error -*phi + 1.0.

Another one is that it requires two match statements rather than one, so -*phi + PI / 2.0 won't be optimized. I'm not sure, but I doubt this will matter in the future for performance.

But, I'm on the fence. Currently, the second match arm would be much more readable with your suggestion. I can implement and use add_param quickly if you prefer. I'm inclined to do it just to look at it.

Implemented your suggestion in 07fec37 (#12662). I'm not 100% sure there isn't a non-negligible efficiency hit. But I think it's easier to read and will be easier to build on.

I see your point, I think we could merge the code as-is and once all Rust standard gates are in place, maybe run the benchmarks with/without add_param to see if there is indeed a significant performance hit.

Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>

coveralls · 2024-06-26T15:14:01Z

Pull Request Test Coverage Report for Build 9681898421

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

17 of 47 (36.17%) changed or added relevant lines in 4 files are covered.
17 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.004%) to 89.724%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/operations.rs	3	33	9.09%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	3	92.37%
qiskit/synthesis/discrete_basis/solovay_kitaev.py	4	94.74%
qiskit/providers/fake_provider/generic_backend_v2.py	10	94.71%

Totals
Change from base Build 9661630219:	0.004%
Covered Lines:	63798
Relevant Lines:	71105

💛 - Coveralls

There is an error in the expression for decomposition of the R gate in the port to Rust. This fixes the error and re-enables the skipped test that failed because of the incorrect expression.

To clone the enum, each variant must be handled separately. This is currently used once, but can be used each time a `Param` is cloned. In case more work needs to be done within match arms, one might choose not to use this function, but rather clone in each of these arms.

This handles `Float` and `ParameterExpression` variants uniformly.

coveralls · 2024-06-27T00:00:25Z

Pull Request Test Coverage Report for Build 9688190770

Details

38 of 49 (77.55%) changed or added relevant lines in 4 files are covered.
27 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.04%) to 89.746%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/operations.rs	24	35	68.57%

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	2	88.2%
crates/qasm2/src/lex.rs	7	92.11%
crates/qasm2/src/parse.rs	18	96.69%

Totals
Change from base Build 9683280067:	-0.04%
Covered Lines:	63828
Relevant Lines:	71121

💛 - Coveralls

ElePT

Thanks a lot @jlapeyre! I see that there might be a trade-off between readability and some performance here, but given that the RGate is used in #12650, I would merge it now to unblock downstream PRs and leave any further optimizations for a follow-up, as I mentioned in my other comment.

* Implement RGate in Rust * Update crates/circuit/src/operations.rs Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com> * Fix error in decomposition of RGate There is an error in the expression for decomposition of the R gate in the port to Rust. This fixes the error and re-enables the skipped test that failed because of the incorrect expression. * Factor cloning the Param enum in Rust To clone the enum, each variant must be handled separately. This is currently used once, but can be used each time a `Param` is cloned. In case more work needs to be done within match arms, one might choose not to use this function, but rather clone in each of these arms. * Run cargo fmt * Implement and use addition for enum Param This handles `Float` and `ParameterExpression` variants uniformly. --------- Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>

Implement RGate in Rust

2a064bd

jlapeyre requested a review from a team as a code owner June 26, 2024 04:59

ElePT reviewed Jun 26, 2024

View reviewed changes

Comment thread crates/circuit/src/operations.rs Outdated

Update crates/circuit/src/operations.rs

a034be5

Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>

mtreinish mentioned this pull request Jun 26, 2024

Use rust gates for Optimize1QGatesDecomposition #12650

Merged

1 task

mtreinish added Changelog: None Do not include in the GitHub Release changelog. mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library performance Rust This PR or issue is related to Rust code in the repository labels Jun 26, 2024

Fix error in decomposition of RGate

7eef0f3

There is an error in the expression for decomposition of the R gate in the port to Rust. This fixes the error and re-enables the skipped test that failed because of the incorrect expression.

jlapeyre mentioned this pull request Jun 26, 2024

Add remaining standard gates to rust representation #12566

Closed

jlapeyre added 4 commits June 26, 2024 17:59

Run cargo fmt

043133f

Implement and use addition for enum Param

07fec37

This handles `Float` and `ParameterExpression` variants uniformly.

Merge branch 'main' into implement-r-gate

bff9fef

ElePT approved these changes Jun 27, 2024

View reviewed changes

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

Merged via the queue into Qiskit:main with commit 6447941 Jun 27, 2024

Conversation

jlapeyre commented Jun 26, 2024

Uh oh!

qiskit-bot commented Jun 26, 2024

Uh oh!

coveralls commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 9673800273

Details

💛 - Coveralls

Uh oh!

ElePT left a comment

Choose a reason for hiding this comment

Uh oh!

ElePT Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

jlapeyre Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

jlapeyre Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

ElePT Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlapeyre Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

jlapeyre Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

jlapeyre Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

ElePT Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 9681898421

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

coveralls commented Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 9688190770

Details

💛 - Coveralls

Uh oh!

ElePT left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Jun 26, 2024 •

edited

Loading

ElePT Jun 26, 2024 •

edited

Loading

coveralls commented Jun 26, 2024 •

edited

Loading

coveralls commented Jun 27, 2024 •

edited

Loading

ElePT left a comment •

edited

Loading