Add inverse function to StandardGate in rust #13168

ShellyGarion · 2024-09-17T14:08:02Z

Summary

Close #13157

Details and comments

qiskit-bot · 2024-09-17T14:08:09Z

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

@Qiskit/terra-core
@kevinhartman
@mtreinish

ShellyGarion · 2024-09-18T08:17:05Z

The following gates do not have an inverse which is a StandardGate:

DCXGate
ISwapGate
CSXGate
RCCXGate
C3SXGate
RC3XGate

coveralls · 2024-09-18T10:24:03Z

Pull Request Test Coverage Report for Build 11236384577

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

151 of 164 (92.07%) changed or added relevant lines in 1 file are covered.
996 unchanged lines in 33 files lost coverage.
Overall coverage increased (+0.09%) to 88.871%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/operations.rs	151	164	92.07%

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/passes/layout/disjoint_utils.py	1	96.83%
qiskit/transpiler/passes/optimization/elide_permutations.py	1	96.0%
qiskit/circuit/library/quantum_volume.py	1	97.44%
crates/accelerate/src/split_2q_unitaries.rs	1	96.0%
crates/accelerate/src/two_qubit_decompose.rs	1	91.45%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	2	88.26%
qiskit/transpiler/passes/optimization/collect_cliffords.py	2	92.59%
crates/circuit/src/imports.rs	2	77.78%
qiskit/circuit/library/standard_gates/u3.py	2	97.03%
qiskit/circuit/library/blueprintcircuit.py	4	96.46%

Totals
Change from base Build 11016050512:	0.09%
Covered Lines:	74746
Relevant Lines:	84106

💛 - Coveralls

mtreinish

Thanks for getting this started it looking good. I have a couple of inline comments and questions to start.

mtreinish · 2024-09-19T15:09:10Z

crates/circuit/src/operations.rs

+    fn inverse(&self, _params: &[Param]) -> Option<(StandardGate, Vec<Param>)> {
+        None
+    }


Is there ever a case where a Python Operation will have an inverse defined? I know it's not part of the abstract interface, but I'm wondering if we should try and fallback to None? @alexanderivrii maybe you have some thoughts here?

Clifford (which is an Operation) does not have an inverse only adjoint.

But there are many generalized gates (like PermutationGate) that have an inverse method

how would you suggest to change the code here?

Following the discussion below, I think for now the main use-case is to add an inverse only for the StandardGates (at least for most of them whose inverse is also a StandardGate)

crates/circuit/src/operations.rs

mtreinish · 2024-09-19T15:17:22Z

crates/circuit/src/operations.rs

@@ -167,6 +167,7 @@ pub trait Operation {
    fn definition(&self, params: &[Param]) -> Option<CircuitData>;
    fn standard_gate(&self) -> Option<StandardGate>;
    fn directive(&self) -> bool;
+    fn inverse(&self, params: &[Param]) -> Option<(StandardGate, Vec<Param>)>;


Does this work in the case of PyGate? I don't think there is a guarantee that a Python defined gate's inverse will be a StandardGate. I actually think it is pretty unlikely in practice, since if a custom gate were expressible as the inverse of a StandardGate it'd probably just be a StandardGate. I think you can just do:

Suggested change

fn inverse(&self, params: &[Param]) -> Option<(StandardGate, Vec<Param>)>;

fn inverse(&self, params: &[Param]) -> Option<(Self, Vec<Param>)>;

then for impl Operation for StandardGate .inverse() returns a StandardGate and for impl Operator for PyGate .inverse() returns a PyGate.

In this case, it's unclear to me which type to put in line 279. Any type I try gives a mismatched type error

We probably can't have Operation::inverse return anything referring to Self, because that's unimplementable by the implementors of Operation that don't have an owned type (e.g. OperationRef). Even within that, it's maybe suboptimal because not all StandardGates would be able to return an inverse, even though they can be inverted (e.g. DCX).

The most general would potentially be to have Operation::inverse return some iterator type over (potentially) PackedInstruction (like CircuitData), but in practice that's probably not super performant for many simple cases. Maybe in the near term, it's enough just to have StandardGate::try_inverse on StandardGate only?

There is a problem in implementing inverse in impl<'a> Operation for OperationRef<'a> (line 279) that cause a mismatched type error. After talking with @jakelishman it seems that the best solution would be to implement inverse only for StandadGate (maybe also for PyGate)

I'm a bit confused now. I can add inverse as a stand-alone function (only for StandardGate) - but then how can I test it from Python ?

pub fn inverse(gate:StandardGate, params: &[Param]) -> Option<(StandardGate, SmallVec<[Param; 3]>)> { match gate { StandardGate::HGate => Some((StandardGate::HGate, smallvec![])), StandardGate::IGate => Some((StandardGate::IGate, smallvec![])), ...

So I only replaced the function name inverse by try_inverse and the function for PyGate will return None.

mtreinish · 2024-09-19T15:20:53Z

crates/circuit/src/operations.rs

+            Self::C3SXGate => None, // the inverse in not a StandardGate
+            Self::RC3XGate => None, // the inverse in not a StandardGate
+        }
+    }
 }


Did you want to add a rust space test asserting that all the StandardGates multiplied with their inverse were the identity? It would be good to get in the practice of having more rust space tests, but since we have coverage from python I don't think it's strictly necessary.

@kevinhartman has been working on demonstrating a pattern of using the py token from rust space tests: #13169 (since we still need it for multiply_param even if there isn't a parameter expression).

I added a test in test_rust_equivalence that asserts that the output of the inverse in Rust (when not None) is the same as the output of the inverse in Python

mtreinish

I know that the current state of the API in this PR was based on previous review suggestions, but I have some concerns about the naming and the exact interface here. I think the goal of making an inverse() method on the operation trait makes a lot of sense, but I have concerns about the current api being called try_inverse and it returning an Option and then never working for anything besides a StandardGate or a OperationRef to a StandardGate. I think if we are going to add this to the Operation trait we should attempt to support all the implementations out there. The only way that makes sense I guess is a Box<dyn Operation> or something similar for the return so that we can dynamically support whatever the inverse implementation returns. I"m not advocating for this though as that adds a layer of pointer indirection for what your immediate use case with StandardGate which is less than desirable.

All of this is to say I think to start instead of adding this to Operation we should just reduce the scope of this PR and define inverse() as a method for StandardGate for now. We can debate what an appropriate interface is for the Operation trait in a separate issue or PR that's not blocking work on something else and then just pivot the StandardGate method to follow that interface (or have the trait method call the StandardGate method inside of it).

test/python/circuit/test_rust_equivalence.py

crates/circuit/src/operations.rs

mtreinish

Thanks for the quick update. This LGTM now with the reduced scope of the new API. I think we can discuss how best to add an inverse method to the wider Operation trait in a follow up. Thinking about how we make the trait's method work will be especially important as we add more types implementing the trait as part of #13264.

add inverse of 1-qubit StatndardGate's

73bc3e2

ShellyGarion added the Rust This PR or issue is related to Rust code in the repository label Sep 17, 2024

ShellyGarion added this to the 1.3.0 milestone Sep 17, 2024

ShellyGarion requested a review from a team as a code owner September 17, 2024 14:08

add inverse of 2-qubit and 3-qubit StandardGate's

64c8de8

ShellyGarion added 2 commits September 18, 2024 04:07

return None if the inverse of a StandardGate is not a StandardGate

e6ad370

add inverse to all impl Operation

db30d87

ShellyGarion added 2 commits September 18, 2024 08:06

update inverse return type

cede375

add test for inverse method

8b0651f

ShellyGarion changed the title ~~[WIP] Add inverse function to Operation in rust~~ Add inverse function to Operation in rust Sep 18, 2024

mtreinish reviewed Sep 19, 2024

View reviewed changes

ShellyGarion added 4 commits September 22, 2024 04:41

replace Vec by SmallVec in Param

4ad079d

fix conflict with main branch

8733a83

update function name inverse to try_inverse

817e488

Merge branch 'main' into inverse_operation_rust

9aa1e34

mtreinish added the Changelog: None Do not include in changelog label Oct 1, 2024

mtreinish reviewed Oct 7, 2024

View reviewed changes

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

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

ShellyGarion mentioned this pull request Oct 8, 2024

Port TwoQubitControlledUDecomposer to rust #13139

Merged

ShellyGarion added 2 commits October 8, 2024 02:45

fix test following review

6154996

replace try_inverse by inverse

e9ab105

ShellyGarion changed the title ~~Add inverse function to Operation in rust~~ Add inverse function to StandardGate in rust Oct 8, 2024

move inverse from Operation to StandardGate

b5a05a8

ShellyGarion requested a review from mtreinish October 8, 2024 16:18

mtreinish approved these changes Oct 8, 2024

View reviewed changes

mtreinish added this pull request to the merge queue Oct 8, 2024

Merged via the queue into Qiskit:main with commit 90e92a4 Oct 8, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add inverse function to StandardGate in rust #13168

Add inverse function to StandardGate in rust #13168

ShellyGarion commented Sep 17, 2024

qiskit-bot commented Sep 17, 2024

ShellyGarion commented Sep 18, 2024 •

edited

Loading

coveralls commented Sep 18, 2024 •

edited

Loading

mtreinish left a comment

mtreinish Sep 19, 2024

ShellyGarion Sep 22, 2024

ShellyGarion Sep 22, 2024

ShellyGarion Sep 22, 2024 •

edited

Loading

ShellyGarion Sep 25, 2024

mtreinish Sep 19, 2024

ShellyGarion Sep 22, 2024

jakelishman Sep 23, 2024

ShellyGarion Sep 23, 2024 •

edited

Loading

ShellyGarion Sep 23, 2024

ShellyGarion Sep 23, 2024 •

edited

Loading

mtreinish Sep 19, 2024

ShellyGarion Sep 22, 2024

mtreinish left a comment

mtreinish left a comment

	fn inverse(&self, params: &[Param]) -> Option<(StandardGate, Vec<Param>)>;
	fn inverse(&self, params: &[Param]) -> Option<(Self, Vec<Param>)>;

Add inverse function to StandardGate in rust #13168

Add inverse function to StandardGate in rust #13168

Conversation

ShellyGarion commented Sep 17, 2024

Summary

Details and comments

qiskit-bot commented Sep 17, 2024

ShellyGarion commented Sep 18, 2024 • edited Loading

coveralls commented Sep 18, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11236384577

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

mtreinish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShellyGarion Sep 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShellyGarion Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShellyGarion Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

ShellyGarion commented Sep 18, 2024 •

edited

Loading

coveralls commented Sep 18, 2024 •

edited

Loading

ShellyGarion Sep 22, 2024 •

edited

Loading

ShellyGarion Sep 23, 2024 •

edited

Loading

ShellyGarion Sep 23, 2024 •

edited

Loading