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

Connectivity constraints for clifford synthesis #506

Open
wants to merge 44 commits into
base: main
Choose a base branch
from

Conversation

JakobSchaeffeler
Copy link

@JakobSchaeffeler JakobSchaeffeler commented Aug 10, 2024

Description

This PR implements connectivity constraints for the clifford synthesis as requested by #328, by passing optional coupling maps. The passed coupling map used to restrict CNOTs to only connected qubits. Additionally, the clifford synthesizer needs to check if the circuit can be synthesized with any permutation of qubits. If no coupling map is passed all to all connectivity is assumed.

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.

Copy link

codecov bot commented Aug 10, 2024

Codecov Report

Attention: Patch coverage is 95.87629% with 8 lines in your changes missing coverage. Please review.

Project coverage is 90.3%. Comparing base (1170b3d) to head (e189af9).

Files with missing lines Patch % Lines
include/cliffordsynthesis/CliffordSynthesizer.hpp 82.3% 3 Missing ⚠️
include/cliffordsynthesis/Results.hpp 81.2% 3 Missing ⚠️
src/cliffordsynthesis/CliffordSynthesizer.cpp 75.0% 1 Missing ⚠️
src/cliffordsynthesis/Tableau.cpp 98.1% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #506     +/-   ##
=======================================
- Coverage   90.4%   90.3%   -0.1%     
=======================================
  Files         87      87             
  Lines      10052   10199    +147     
  Branches    1722    1759     +37     
=======================================
+ Hits        9093    9217    +124     
- Misses       959     982     +23     
Flag Coverage Δ
cpp 90.0% <95.5%> (-0.2%) ⬇️
python 96.8% <100.0%> (+0.8%) ⬆️
Files with missing lines Coverage Δ
include/cliffordsynthesis/Tableau.hpp 86.6% <ø> (ø)
include/cliffordsynthesis/encoding/GateEncoder.hpp 90.0% <100.0%> (+1.1%) ⬆️
include/cliffordsynthesis/encoding/SATEncoder.hpp 100.0% <ø> (ø)
...lude/cliffordsynthesis/encoding/TableauEncoder.hpp 100.0% <ø> (ø)
src/cliffordsynthesis/encoding/GateEncoder.cpp 100.0% <100.0%> (ø)
...rc/cliffordsynthesis/encoding/MultiGateEncoder.cpp 100.0% <100.0%> (ø)
src/cliffordsynthesis/encoding/SATEncoder.cpp 83.7% <100.0%> (+0.3%) ⬆️
...c/cliffordsynthesis/encoding/SingleGateEncoder.cpp 100.0% <100.0%> (ø)
src/cliffordsynthesis/encoding/TableauEncoder.cpp 91.0% <100.0%> (+4.2%) ⬆️
src/mqt/qmap/clifford_synthesis.py 100.0% <100.0%> (+5.2%) ⬆️
... and 4 more

... and 4 files with indirect coverage changes

@JakobSchaeffeler JakobSchaeffeler marked this pull request as ready for review August 14, 2024 09:09
@burgholzer
Copy link
Member

Note

#507 just went in and brought some updates from the MQT Core dependency that cause some conflicts here. Could you please resolve these? They should be fairly easy to resolve. Sorry for the inconvenience.

@JakobSchaeffeler
Copy link
Author

@burgholzer I tried to resolve the conflicts and everything works locally. However the CI/Change/Check test fails, but all subtasks in there are marked as successful. Do you know what the issue is?

@burgholzer burgholzer added feature New feature or request python Anything related to Python code c++ Anything related to C++ code minor Changes leading to a minor version increase labels Aug 14, 2024
@burgholzer
Copy link
Member

@burgholzer I tried to resolve the conflicts and everything works locally. However the CI/Change/Check test fails, but all subtasks in there are marked as successful. Do you know what the issue is?

Thanks for fixing the conflicts so quickly.
I just retriggered the respective CI job. Let's see if it still fails on the second try.
Should be unrelated to your changes in any case.

@pehamTom pehamTom self-assigned this Aug 19, 2024
Copy link
Member

@pehamTom pehamTom left a comment

Choose a reason for hiding this comment

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

Very nice work!

I actually do not have too much to comment on this. The only thing that could be nicer from a maintainability standpoint is to get rid of the offsets in the encodings of every constraint. That might require some more involved rewrites. Unless you have a quick and elegant idea to get rid of this, we can leave it for future refactoring.

One additional feature that would go a long way is to also include a small example for this functionality in the documentation. Extending the Jupyter Notebook for https://mqt.readthedocs.io/projects/qmap/en/latest/Synthesis.html by a small example should be sufficient.

include/cliffordsynthesis/Results.hpp Outdated Show resolved Hide resolved
src/cliffordsynthesis/Tableau.cpp Outdated Show resolved Hide resolved
src/cliffordsynthesis/Tableau.cpp Outdated Show resolved Hide resolved
src/cliffordsynthesis/Tableau.cpp Outdated Show resolved Hide resolved
src/cliffordsynthesis/Tableau.cpp Outdated Show resolved Hide resolved
src/cliffordsynthesis/encoding/TableauEncoder.cpp Outdated Show resolved Hide resolved
@JakobSchaeffeler
Copy link
Author

@burgholzer @pehamTom I incorporated the feedback by Tom. However now all Python Test fail with a Deprecation warning and one C test which should be unrelated to my PR. Do you know how to solve that?

@pehamTom
Copy link
Member

@burgholzer @pehamTom I incorporated the feedback by Tom. However now all Python Test fail with a Deprecation warning and one C test which should be unrelated to my PR. Do you know how to solve that?

Yeah, that is not your fault. We leave the PR open until the IDP is over anyway. I will take care of it.

@burgholzer
Copy link
Member

@burgholzer @pehamTom I incorporated the feedback by Tom. However now all Python Test fail with a Deprecation warning and one C test which should be unrelated to my PR. Do you know how to solve that?

I think if you hit the "Update Branch" button (effectively merging in the main branch), the deprecation warnings should be gone. IIRC, I fixed them for QMAP in one of the latest updates.
Otherwise, I'll probably address them next week in a separate PR. They should not be related to your changes here 😌

@burgholzer
Copy link
Member

Just merged the main branch in which contains the fixes from #509.
The deprecation warnings here are now really related to your code changes and should be addressed (should be fairly easy).

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 feature New feature or request minor Changes leading to a minor version increase python Anything related to Python code
Projects
Status: In Progress
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants