Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add user choice between U3 or GPI2 as single-qubit native gate #1095

Merged
merged 12 commits into from
Nov 27, 2023

Conversation

Simone-Bordoni
Copy link
Contributor

@Simone-Bordoni Simone-Bordoni commented Nov 15, 2023

Make the user choose between U3 or GPI2 as single qubit native gate

Checklist:

  • Reviewers confirm new code works as expected.
  • Tests are passing.
  • Coverage does not decrease.
  • Documentation is updated.

@Simone-Bordoni
Copy link
Contributor Author

For now i am keeping both u3 decomposition and add the gpi2 decomposition. As the hard-coded gates decompositions are growing a lot i have decided to put them in a separate file.
Having a general u3 decomposition may be a good idea to implement future more genmeral decomposition by choosing all single qubit native gates. Let me know what you think, if you agree with this implementation i will proceed by hard-coding all gpi2 decomposition.
@stavros11 @BrunoLiegiBastonLiegi

@Simone-Bordoni Simone-Bordoni marked this pull request as ready for review November 22, 2023 12:22
Copy link

codecov bot commented Nov 23, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5c4b4b2) 100.00% compared to head (c9b57a3) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #1095   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           63        64    +1     
  Lines         8906      8943   +37     
=========================================
+ Hits          8906      8943   +37     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/qibo/transpiler/abstract.py Outdated Show resolved Hide resolved
src/qibo/transpiler/decompositions.py Outdated Show resolved Hide resolved
)

# register the iSWAP decompositions
iswap_dec = GateDecompositions()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why for 2-qubits gates the decompositions are not restricted to the native set anymore? I mean I understand that the decompositions of the non-native gates are provided by gpi2_dec and u3_dec but why not to write them in their u3/gpi2 decomposition directly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for two-qubits we are doing two passes, the first is converting the arbitrary two-qubit gate to arbitrary single-qubit + native two-qubit and the second converts the single-qubits to native.
In terms of code this happens here

decomposition_2q = _translate_two_qubit_gates(gate, native_gates)

and the following lines.

That being said, I think we need a more strict mapping between the decompositions and what native gates each decomposition assumes. For example the u3_dec translates almost everything to U3 but still assumes that Z and RZ are natives. Similarly the iswap_dec still assumes that a bunch of single-qubit gates (that are not iSWAPs) are native gates.
We need to come up with a better design where u3_dec translates everything to U3, but if the user passes NativeGates.RZ | NativeGates.U3 to the Unroller, it should be smart enough to use both and get the optimal decompositions.

src/qibo/transpiler/decompositions.py Show resolved Hide resolved
Copy link
Member

@stavros11 stavros11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Simone-Bordoni, @BrunoLiegiBastonLiegi for looking into this. Generally, the mechanism of plugging native gates to the unroller looks good, except a few comments below. I also agree with moving the decompositions to a different file.

The only issue I see is that our current mechanism of decompositions and the way the unroller functions (_translate_* etc.) use the decompositions is not very scalable if we attempt to use them with different combinations of native gates and the code could also be simplified. Unfortunately I cannot think of a better proposal but if I come up with something or if you have any suggestions we can discuss. I have the impression that some kind of "inheritance" between decompositions could help, but I may be wrong.

src/qibo/transpiler/abstract.py Outdated Show resolved Hide resolved
src/qibo/transpiler/abstract.py Outdated Show resolved Hide resolved
src/qibo/transpiler/abstract.py Outdated Show resolved Hide resolved
src/qibo/transpiler/unroller.py Outdated Show resolved Hide resolved
src/qibo/transpiler/unroller.py Outdated Show resolved Hide resolved
src/qibo/transpiler/unroller.py Outdated Show resolved Hide resolved
@Simone-Bordoni
Copy link
Contributor Author

Thanks @Simone-Bordoni, @BrunoLiegiBastonLiegi for looking into this. Generally, the mechanism of plugging native gates to the unroller looks good, except a few comments below. I also agree with moving the decompositions to a different file.

The only issue I see is that our current mechanism of decompositions and the way the unroller functions (_translate_* etc.) use the decompositions is not very scalable if we attempt to use them with different combinations of native gates and the code could also be simplified. Unfortunately I cannot think of a better proposal but if I come up with something or if you have any suggestions we can discuss. I have the impression that some kind of "inheritance" between decompositions could help, but I may be wrong.

Maye tommorrow we should discuss better this point. I agree that scalability is an issue with this implementation but I don't have any ideas of how to solve the problem.

@Simone-Bordoni
Copy link
Contributor Author

I have finisched correcting the "small issues" requested in the review. I think, once reviewed again, we should merge this PR in order to have a working decomposition with GPI2. In the future we can try to address the problem of general decompositions when we will have more ideas.

Copy link
Member

@stavros11 stavros11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with merging as this is a good simplification. If you can fix the coverage this should be ready.

@renatomello renatomello changed the title U3 to gpi2 Add user choice between U3 or GPI2 as single-qubit native gate Nov 27, 2023
@renatomello renatomello merged commit e62a6b9 into master Nov 27, 2023
21 checks passed
@renatomello renatomello deleted the u3_to_gpi2 branch November 27, 2023 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants