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 strip idle qubits #394

Merged
merged 19 commits into from
May 23, 2024
Merged

Conversation

TeWas
Copy link
Contributor

@TeWas TeWas commented May 8, 2024

Description

This PR fixes the issue of incorrectly removing idle qubits that are not idle in both circuits.
The following changes have been made:

  • Previously, idle qubits were removed regardless of their status in both circuits. Now, an idle qubit is removed only if:
    • it is idle in both circuits
    • it is idle in one circuit and does not exist in the other circuit
  • To implement this logic, a dedicated method stripIdleQubits has been created in QCEC. This method considers both circuits in conjunction
  • The stripIdleQubits method:
    • examines each logical qubit in the initialLayout of the larger circuit
    • checks if the corresponding physical qubit is idle
    • then searches for this logical qubit in the other circuit
    • if the corresponding physical qubit in the other circuit is also idle, then the logical and corresponding physical qubit are removed from each circuit, respectively, under the following conditions:
      • neither the logical nor the physical qubit appears in the output permutation (applies to both circuits)
      • or if the initial and output permutations match for this qubit (applies to both circuits)
    • if an idle logical qubit is present in one circuit only, it is removed, if neither the logical nor the physical qubit appears in the output permutation, or the initial and output permutations match for this qubit

Fixes #372

Checklist:

  • The pull request only contains commits that are related to it.
  • I have added appropriate tests and documentation.
  • I have made sure that all CI jobs on GitHub pass.
  • The pull request introduces no new warnings and follows the project's style guidelines.

TeWas added 5 commits May 2, 2024 14:15
* Test qubit idle in both circuits
* Test qubit idle in one circuit only
* Test that qubits are not removed individually anymore
@TeWas TeWas marked this pull request as draft May 8, 2024 14:54
Copy link

codecov bot commented May 8, 2024

Codecov Report

Attention: Patch coverage is 97.08738% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 96.4%. Comparing base (cc1d2c3) to head (cd0898e).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #394     +/-   ##
=======================================
- Coverage   96.5%   96.4%   -0.2%     
=======================================
  Files         35      35             
  Lines       1761    1818     +57     
  Branches     219     224      +5     
=======================================
+ Hits        1701    1754     +53     
- Misses        60      64      +4     
Flag Coverage Δ
cpp 96.2% <97.0%> (-0.2%) ⬇️
python 97.2% <100.0%> (+<0.1%) ⬆️
Files Coverage Δ
include/Configuration.hpp 0.0% <ø> (ø)
include/EquivalenceCheckingManager.hpp 100.0% <100.0%> (ø)
src/Configuration.cpp 100.0% <ø> (ø)
src/checker/zx/ZXChecker.cpp 100.0% <100.0%> (ø)
src/mqt/qcec/compilation_flow_profiles.py 97.5% <100.0%> (+<0.1%) ⬆️
src/mqt/qcec/configuration.py 100.0% <ø> (ø)
src/EquivalenceCheckingManager.cpp 93.8% <98.7%> (+0.2%) ⬆️
...dd/applicationscheme/GateCostApplicationScheme.hpp 91.8% <71.4%> (-3.7%) ⬇️

... and 2 files with indirect coverage changes

@burgholzer burgholzer added c++ Anything related to C++ code fix Anything related to bugfixes labels May 17, 2024
@burgholzer burgholzer marked this pull request as ready for review May 19, 2024 14:50
Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

Hey 👋

Many thanks for working on this! The current PR is already a great start. Thanks for the thorough tests as well as the detailed description of the changes! The two inline comments below just contain a couple of suggestions on how to further improve the overall contribution.

I also took the liberty of implementing a couple of changes myself, which I am going to push right after this review. I mostly refactored the logic of the idle qubit removal a little bit to need fewer levels of indentation. The major change is to how you implemented the functionality is that an idle qubit is now also removed if it is not contained in one of the output permutations, but contained yet identical to the initial layout in the other. I believe there is no need to enforce that the same removal condition holds in both circuits. That should give a little more flexibility.

I will also push a fix in the ZX Checker that was revealed by the idle qubit stripping changes. Turns out the permutation handling there wasn't ideal yet which led to an exception being thrown in the BV verification tests. Should be fixed now.

With these changes, the CI will hopefully be all green 🚀

include/EquivalenceCheckingManager.hpp Outdated Show resolved Hide resolved
include/EquivalenceCheckingManager.hpp Outdated Show resolved Hide resolved
…on that is not exposed

Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
also adds `const` where appropriate to silence clang-tidy

Signed-off-by: burgholzer <burgholzer@me.com>
previously, this would not work properly if the set of physical qubits in the permutation (the keys) would not be identical to the set of logical qubits (the keys).

Signed-off-by: burgholzer <burgholzer@me.com>
burgholzer and others added 7 commits May 19, 2024 17:11
Copy link
Contributor Author

@TeWas TeWas left a comment

Choose a reason for hiding this comment

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

Hi,
thanks for the structural improvements, LGTM 👍
And I implemented the remaining changes!

@TeWas TeWas requested a review from burgholzer May 21, 2024 10:37
Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

This LGTM overall. Many thanks for the additional changes. I'll try to quickly resolve the remaining failing CI check tomorrow. It's just a coincidence that those profile files are identical when produced by the new Qiskit release. This might change in the future and might require a small workaround that adds an identifier in the files themselves.

Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
@burgholzer burgholzer merged commit 1358f62 into cda-tum:main May 23, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Anything related to C++ code fix Anything related to bugfixes
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

🐛 Incorrect stripping of idle qubits that are not idle in both circuits
2 participants