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

Move gates.Gate.decompose to transpiler.decompositions #1188

Merged
merged 12 commits into from
Feb 7, 2024

Conversation

renatomello
Copy link
Contributor

@renatomello renatomello commented Feb 3, 2024

Closes #1040

Checklist:

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

@renatomello renatomello added this to the Qibo 0.2.5 milestone Feb 3, 2024
@renatomello renatomello self-assigned this Feb 3, 2024
Copy link

codecov bot commented Feb 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2bfd6ec) 100.00% compared to head (2a7b55b) 100.00%.
Report is 2 commits behind head on master.

❗ Current head 2a7b55b differs from pull request most recent head a155aad. Consider uploading reports for the commit a155aad to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #1188   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           69        69           
  Lines        10107     10124   +17     
=========================================
+ Hits         10107     10124   +17     
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.

@renatomello renatomello marked this pull request as ready for review February 4, 2024 05:45
Copy link
Contributor

@Simone-Bordoni Simone-Bordoni left a comment

Choose a reason for hiding this comment

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

Thank you for moving the Gate.decompose to the transpiler decompositions. This is a good start for the next step of having a general decomposer. I plan on starting it next week.

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 @renatomello, I believe this is a step in the right direction since it is reducing code duplication (or at least puts all decompositions in a single place). I have some questions:

  1. Do we still the Gate.decompose interface? If this is not used anywhere I would consider dropping completely. This way we avoid the ugly pylint exceptions and we don't have to maintain two interfaces for the same thing - we also force the users to use the transpiler interface directly. Personally I am not aware of any applications of Gate.decompose.
  2. Is there something special about the standard_decomposition? What makes it more "standard" than the other decompositions?

Not necessarily for this PR, but in my mind it would be useful have a map or function in which we pass the available native gates and we get back a GateDecomposition that decomposes everything to these gates, for example

get_decomposition(NativeGates.GPI2 | NativeGates.CZ) -> cz_dec

etc. Then there is a trickier part of whether multiple decompositions can be combined to reach the desired gates, but that we could even leave for a later step.

@renatomello
Copy link
Contributor Author

renatomello commented Feb 6, 2024

1. Do we still the `Gate.decompose` interface? If this is not used anywhere I would consider dropping completely. This way we avoid the ugly pylint exceptions and we don't have to maintain two interfaces for the same thing - we also force the users to use the transpiler interface directly. Personally I am not aware of any applications of `Gate.decompose`.

I would say yes. I, for one, sometimes want to get a decomposition of one specific gate without being in the context of transpiling an entire circuit. It'd be annoying to have to create a circuit, add the gate, and then transpile the circuit just to get the decomposition of one gate.

2. Is there something special about the `standard_decomposition`? What makes it more "standard" than the other decompositions?

No. Just like with any "standard", it is, by definition, arbitrary. In this case it just means the ones that were already hard-coded in the decompose methods and were the ones being used by default.

@renatomello renatomello requested a review from stavros11 February 6, 2024 04:40
@codecov-commenter
Copy link

codecov-commenter commented Feb 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5eb7367) 100.00% compared to head (afa52c7) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #1188   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           68        68           
  Lines        10203     10215   +12     
=========================================
+ Hits         10203     10215   +12     
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.

@stavros11
Copy link
Member

I would say yes. I, for one, sometimes want to get a decomposition of one specific gate without being in the context of transpiling an entire circuit. It'd be annoying to have to create a circuit, add the gate, and then transpile the circuit just to get the decomposition of one gate.

You can already do that using just

from qibo.transpiler.decompositions import standard_decompositions

dec_gate =  standard_decompositions(gate)

without having to create any circuits. This is also more flexible because you can use any decomposition in the same fashion, not only the "standard" one.

If the main issue is the names and import, we could propose better names and arrange the modules so that you can simply do

from qibo.transpiler import mydecomposition

or

from qibo.decompositions import mydecomposition

@Simone-Bordoni
Copy link
Contributor

I think that the best way is either to use only the "transpiler method" or to allow in the decompose method to decide the basis gates. However as a first implementation only to store the decompositions in a single place this should be enough. Why don't we discuss the structure of the general unroller in person at the beginning of next week?

@renatomello
Copy link
Contributor Author

renatomello commented Feb 6, 2024

I think that the best way is either to use only the "transpiler method" or to allow in the decompose method to decide the basis gates. However as a first implementation only to store the decompositions in a single place this should be enough. Why don't we discuss the structure of the general unroller in person at the beginning of next week?

I agree with this. Is it ok for you @stavros11?

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 still don't see how Gate.decompose itself can be useful but okay.

@MatteoRobbiati MatteoRobbiati added this pull request to the merge queue Feb 7, 2024
Merged via the queue into master with commit 40a1afe Feb 7, 2024
19 checks passed
@stavros11 stavros11 deleted the decompositions branch February 17, 2024 09:13
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.

Move Gate.decompose() methods to transpiler
5 participants