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

Fix transpiler bugs about moved measurements and duplicated gates #1369

Merged
merged 22 commits into from
Jun 28, 2024

Conversation

Simone-Bordoni
Copy link
Contributor

@Simone-Bordoni Simone-Bordoni commented Jun 21, 2024

Solve bugs in #1363 and #1318

Checklist:

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

@Simone-Bordoni
Copy link
Contributor Author

@BrunoLiegiBastonLiegi I have solved the bug reported in #1363. I will try to rapidly fix the Issue in #1318, in the meantime you can start the review of this part (I was not reardering the initial qubit map and this created some issues with the random placer). I have also added the seed for the random placer

Copy link

codecov bot commented Jun 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.84%. Comparing base (88825fa) to head (d78b626).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1369   +/-   ##
=======================================
  Coverage   99.84%   99.84%           
=======================================
  Files          75       75           
  Lines       10799    10807    +8     
=======================================
+ Hits        10782    10790    +8     
  Misses         17       17           
Flag Coverage Δ
unittests 99.84% <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.

@Simone-Bordoni Simone-Bordoni marked this pull request as ready for review June 21, 2024 13:25
@renatomello
Copy link
Contributor

@Simone-Bordoni Can the PR name be more descriptive?

@Simone-Bordoni Simone-Bordoni changed the title Fix transpiler bugs Fix transpiler bugs #1363 and #1318 Jun 25, 2024
@Simone-Bordoni Simone-Bordoni changed the title Fix transpiler bugs #1363 and #1318 Fix transpiler bugs about moved measurements and duplicated gates Jun 25, 2024
src/qibo/transpiler/router.py Outdated Show resolved Hide resolved
tests/test_transpiler_pipeline.py Outdated Show resolved Hide resolved
tests/test_transpiler_unroller.py Show resolved Hide resolved
Copy link
Contributor

@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi 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 I tested this with #1106 and it seems to be working

@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi mentioned this pull request Jun 26, 2024
5 tasks
Copy link
Contributor

@renatomello renatomello left a comment

Choose a reason for hiding this comment

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

A couple of other things:

  1. the function translate_gate in unroller.py receives the variable gate as an input, but redefines what gate is in a loop.
  2. the Custom.__call__ method in placer.py receives the variable map as an input, but map is a python built-in function. This may lead to issues, so this variable should be renamed.

Copy link
Contributor

@andrea-pasquale andrea-pasquale 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.
I can confirm that this PR fixes #1318.
While testing the state_tomography in Qibocal, I noticed that even without this PR is now failing with this error:

File "/home/andreapasquale/qibocal/src/qibocal/protocols/state_tomography.py", line 114, in _acquisition
    _, results = execute_transpiled_circuit(
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/andreapasquale/qibocal/src/qibocal/auto/transpile.py", line 60, in execute_transpiled_circuit
    transpiled_circ, _ = transpiler(new_circuit)
                         ^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/andreapasquale/qibo/src/qibo/transpiler/pipeline.py", line 265, in __call__
    return circuit, final_layout
                    ^^^^^^^^^^^^
UnboundLocalError: cannot access local variable 'final_layout' where it is not associated with a value

I believe that the error was introduced by 8d29397. It should be related to the fact that we are returning final_layout even if it is defined only in a if statement. Probably in qibocal we fall in one of the cases where final_layout is not defined.
I believe that this was related to the changes that @Edoardo-Pedicillo introduced in #1301.
Given that the error by itself is not related to this PR feel free to address the problem here or in a separate PR and I will open an issue pointing to this comment.

src/qibo/transpiler/placer.py Outdated Show resolved Hide resolved
@renatomello renatomello added this pull request to the merge queue Jun 28, 2024
@renatomello renatomello added this to the Qibo 0.2.9 milestone Jun 28, 2024
Merged via the queue into master with commit 9161904 Jun 28, 2024
27 checks passed
@renatomello renatomello deleted the fix_transpiler_bugs branch June 28, 2024 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working transpiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transpiler moving measurements on wrong qubit Unroller duplicates gates when measuring in specific basis
4 participants