Add rust representation for the u1, u2, and u3 gates #12572

mtreinish · 2024-06-13T13:11:11Z

Summary

This commit adds the rust representation of the U1Gate, U2Gate, and U3Gate to the StandardGates enum in rust.

Details and comments

Part of #12566

qiskit-bot · 2024-06-13T13:11:17Z

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

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

Cryoris

Overall LGTM, just two questions from my side 🙂

crates/circuit/src/operations.rs

test/python/circuit/test_rust_equivalence.py

ElePT · 2024-06-18T12:18:44Z

crates/circuit/src/operations.rs

@@ -374,6 +382,23 @@ impl Operation for StandardGate {
                }
                _ => None,
            },
+            Self::U1Gate => match params[0] {
+                Param::Float(val) => Some(aview2(&gate_matrix::u1_gate(val)).to_owned()),


I've noticed that &gate_matrix::u1_gate(val) is not dereferencing val, while the other gate calls do: &gate_matrix::u2_gate(*phi, *lam), u3_gate(*theta, *phi, *lam). Was this intentional? (aha, it's because it's just a float, never mind).

The difference here is that the match is on params[0] instead of params. The val is already an f64 and you can't dereference that, while for the other gates the angles have &f64.

I haven't dug into the test failures yet, but I suspect it's more likely I screwed up something in the translation. The qasm3 failure at least is because there are no longer fixed object ids for the gates and I need to update the test to account for that (i.e. use a regex).

If this is a blocker for #12598 feel free to just rebase it on main and move ahead without this. I can reindex the gates in this PR without too much trouble.

I fixed the test failures in: 65ff009 the optimize 1q gates decomposition failures were because the tests were doing:

gate.definition.global_phase = val

and that doesn't do anything to the gate object for a standard gate in rust. TBH, I have no idea what this test is trying to do but to fix it I just used a custom gate class instead of U1Gate.

So gate.definition.global_phase = val is another of the "hidden forbidden moves", like dag_node.op.params[0]. Do you think we should document it in the reno too? At a first glance people would probably not expect it to fail.

I'm not sure it needs one, the definition was never really supposed to be mutable like this as it's an inherent attribute of a gate, changing it doesn't make much sense (for most things it doesn't get used either). This already wouldn't work with singletons as changing the global phase would affect every instance of the gate. I viewed this as test bug, because I have no idea what that test was trying to accomplish. But I can add a release note out of an abundance of caution if you think it's warranted here.

Hmmm I think it would be nice to have it documented somewhere, but I am ok just keeping an eye on it, I don't think people refer to the release notes anywhere and I see how documenting every edge case can be a problem.

crates/circuit/src/imports.rs

This commit adds the rust representation of the U1Gate, U2Gate, and U3Gate to the `StandardGates` enum in rust. Part of Qiskit#12566

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

coveralls · 2024-06-21T19:11:57Z

Pull Request Test Coverage Report for Build 9618403783

Details

82 of 85 (96.47%) changed or added relevant lines in 5 files are covered.
11 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.008%) to 89.786%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/operations.rs	50	53	94.34%

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	2	88.2%
crates/qasm2/src/lex.rs	3	92.88%
crates/qasm2/src/parse.rs	6	97.61%

Totals
Change from base Build 9616513556:	0.008%
Covered Lines:	63739
Relevant Lines:	70990

💛 - Coveralls

coveralls · 2024-06-21T19:49:23Z

Pull Request Test Coverage Report for Build 9618804762

Details

82 of 85 (96.47%) changed or added relevant lines in 5 files are covered.
7 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.01%) to 89.792%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/operations.rs	50	53	94.34%

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	2	88.2%
crates/qasm2/src/lex.rs	5	92.37%

Totals
Change from base Build 9616513556:	0.01%
Covered Lines:	63743
Relevant Lines:	70990

💛 - Coveralls

ElePT

The implementation LGTM, I just had a couple of questions about the cases that triggered test failures.

ElePT · 2024-06-24T08:48:16Z

test/python/circuit/test_rust_equivalence.py

@@ -102,7 +113,7 @@ def test_matrix(self):
                continue

            with self.subTest(name=name):
-                params = [pi] * standard_gate._num_params()
+                params = [0.1] * standard_gate._num_params()


Was this change because math.pi was triggering some of the test failures? I feel like it's commonly used for assigning parameter values.

Oh, I thought I changed it back. While debugging I did this because one of the test failures was using 0.1 as the angle value and I wanted to rule that out as the cause of the failure (checking the matrix was also correct with 0.1). I can change it back to pi, it realistically doesn't matter too much though.

I'd even expect 0.1 to be a better value for testing since for pi the matrices take on some special forms and 0.1 is more "irregular" in that sense 🤔

Sounds good, I just wanted to make sure we were not ignoring pi.

ElePT · 2024-06-24T08:53:07Z

crates/circuit/src/operations.rs

@@ -374,6 +382,23 @@ impl Operation for StandardGate {
                }
                _ => None,
            },
+            Self::U1Gate => match params[0] {
+                Param::Float(val) => Some(aview2(&gate_matrix::u1_gate(val)).to_owned()),


So gate.definition.global_phase = val is another of the "hidden forbidden moves", like dag_node.op.params[0]. Do you think we should document it in the reno too? At a first glance people would probably not expect it to fail.

* Add rust representation for the u1, u2, and u3 gates This commit adds the rust representation of the U1Gate, U2Gate, and U3Gate to the `StandardGates` enum in rust. Part of Qiskit#12566 * Update crates/circuit/src/imports.rs Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com> * Fix test failures * Fix pylint pedantry --------- Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>

mtreinish added performance Changelog: None Do not include in changelog Rust This PR or issue is related to Rust code in the repository labels Jun 13, 2024

mtreinish added this to the 1.2.0 milestone Jun 13, 2024

mtreinish requested a review from a team as a code owner June 13, 2024 13:11

mtreinish mentioned this pull request Jun 13, 2024

Add remaining standard gates to rust representation #12566

Closed

mtreinish force-pushed the add-u123-to-rs branch from dce2b20 to afad78d Compare June 14, 2024 04:59

Cryoris reviewed Jun 14, 2024

View reviewed changes

crates/circuit/src/operations.rs Outdated Show resolved Hide resolved

test/python/circuit/test_rust_equivalence.py Show resolved Hide resolved

ElePT mentioned this pull request Jun 18, 2024

Add rust representation for SGates, TGates, and iSwap gate #12598

Merged

ElePT reviewed Jun 18, 2024

View reviewed changes

crates/circuit/src/imports.rs Outdated Show resolved Hide resolved

mtreinish and others added 3 commits June 21, 2024 13:38

Add rust representation for the u1, u2, and u3 gates

da90e25

This commit adds the rust representation of the U1Gate, U2Gate, and U3Gate to the `StandardGates` enum in rust. Part of Qiskit#12566

Update crates/circuit/src/imports.rs

5c80610

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

Fix test failures

65ff009

mtreinish force-pushed the add-u123-to-rs branch from 53fa181 to 65ff009 Compare June 21, 2024 18:48

Fix pylint pedantry

306bd54

mtreinish requested a review from ElePT June 21, 2024 21:56

ElePT approved these changes Jun 24, 2024

View reviewed changes

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

Merged via the queue into Qiskit:main with commit bf8f398 Jun 24, 2024

mtreinish deleted the add-u123-to-rs branch June 24, 2024 18:26

Add rust representation for the u1, u2, and u3 gates #12572

Add rust representation for the u1, u2, and u3 gates #12572

Uh oh!

Conversation

mtreinish commented Jun 13, 2024

Summary

Details and comments

Uh oh!

qiskit-bot commented Jun 13, 2024

Uh oh!

Cryoris left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ElePT Jun 18, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 9618403783

Details

💛 - Coveralls

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 9618804762

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ElePT Jun 18, 2024 •

edited

Loading

coveralls commented Jun 21, 2024 •

edited

Loading

coveralls commented Jun 21, 2024 •

edited

Loading

ElePT left a comment •

edited

Loading