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 gates.CNOT as a native gate in gate decomposition #1422

Merged
merged 6 commits into from
Aug 28, 2024

Conversation

csookim
Copy link
Member

@csookim csookim commented Aug 19, 2024

This covers issue #1421.

Checklist:

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

@renatomello renatomello changed the title Add gates.CNOT as a native gate Add gates.CNOT as a native gate in gate decomposition Aug 20, 2024
@renatomello renatomello added transpiler enhancement New feature or request labels Aug 20, 2024
@qiboteam qiboteam deleted a comment from codecov bot Aug 20, 2024
@csookim
Copy link
Member Author

csookim commented Aug 20, 2024

Example

circ = Circuit(2)
circ.add(gates.H(0))
circ.add(gates.CNOT(0, 1))
circ.add(gates.SWAP(0, 1))
circ.add(gates.CZ(0, 1))
circ.add(gates.M(0, 1))

gate_list = [gates.GPI2, gates.RZ, gates.Z, gates.M, gates.CNOT]
native_gates = NativeGates(0).from_gatelist(gate_list)

custom_pipeline = Passes([Unroller(native_gates=native_gates)])
unrolled_circuit, _ = custom_pipeline(circ)

print(unrolled_circuit.draw(line_wrap=100))

Output

q0: ─ZGPI2ooXo────────o────────Mq1: ────────XXoXZGPI2XZGPI2M

@csookim csookim marked this pull request as ready for review August 20, 2024 07:13
Copy link
Member

@alecandido alecandido left a comment

Choose a reason for hiding this comment

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

As said in the issue, the unroller should be entirely reviewed, since there are many more combinations that now are not accounted for.

In any case, they may also be unrealistic, and support is not needed in the short term.

For the time being, just fix the docstrings, and it should be ready to go.

src/qibo/transpiler/unroller.py Outdated Show resolved Hide resolved
src/qibo/transpiler/unroller.py Show resolved Hide resolved
@qiboteam qiboteam deleted a comment from codecov bot Aug 26, 2024
Copy link
Member

@alecandido alecandido left a comment

Choose a reason for hiding this comment

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

@csookim, you didn't have to ask again for a review: I already approved, and the modifications I asked were so minor that I trusted you on how to implement them :)

@alecandido
Copy link
Member

alecandido commented Aug 26, 2024

Btw, why are you deleting Codecov comments? Just out of curiosity
(the checks in the workflows are still there; so it's just a different in appearance, but no substantial change, so it's even fine)

image

Copy link

codecov bot commented Aug 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.94%. Comparing base (8fb9af9) to head (3281871).
Report is 24 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1422   +/-   ##
=======================================
  Coverage   99.94%   99.94%           
=======================================
  Files          78       78           
  Lines       11297    11304    +7     
=======================================
+ Hits        11291    11298    +7     
  Misses          6        6           
Flag Coverage Δ
unittests 99.94% <100.00%> (+<0.01%) ⬆️

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.

@csookim
Copy link
Member Author

csookim commented Aug 26, 2024

Btw, why are you deleting Codecov comments? Just out of curiosity (the checks in the workflows are still there; so it's just a different in appearance, but no substantial change, so it's even fine)

image

I deleted it to avoid confusion, as the removed one was the coverage of the previous code. Will it be updated automatically?

@alecandido
Copy link
Member

I deleted it to avoid confusion, as the removed one was the coverage of the previous code. Will it be updated automatically?

Indeed, if you leave it there it will edit the same comment, instead of adding a new one :)

# No CZ, iSWAP gates in the native gate set
# Decompose CNOT, CZ, SWAP gates into CNOT gates
if native_gates & NativeGates.CNOT:
return cnot_dec_temp(gate)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can make this decomposition work for every gate by first decomposing into CZ and then decomposing CZ into CNOT. This is how it works for iSWAP decomposition (line 238-250).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but it adds two H gates next to the CNOT and it requires optimization (fusion) of 1q gates. Since Paul will use simple circuits for testing, I only added basic decompositions. As @alecandido mentioned, it would be better to entirely review the unroller.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be a realy easy check to remove two successive H gates, however if you need it only for simple applications it is fine

@Simone-Bordoni Simone-Bordoni self-requested a review August 26, 2024 09:49
@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi added this pull request to the merge queue Aug 28, 2024
Merged via the queue into master with commit ecbd16d Aug 28, 2024
27 checks passed
@alecandido alecandido deleted the unroller_cnot branch August 28, 2024 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request transpiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants