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 State Preparation Algorithm #543

Merged
merged 101 commits into from
Feb 21, 2025
Merged

Conversation

M-J-Hochreiter
Copy link
Contributor

@M-J-Hochreiter M-J-Hochreiter commented Jan 31, 2024

Description

As a part of my bachelor thesis I implemented the Qiskit State Preparation algorithm (https://github.com/Qiskit/qiskit/blob/main/qiskit/circuit/library/data_preparation/state_preparation.py#) in C++ using the MQT Framework.
My advisor Yannick Stade suggested I open a pull request, as it might be interesting to add into MQT.

The state preparation allows a user to create a circuit that initializes a quantum state from a list of complex amplitudes, sometime needed in various quantum algorithms.

I am not sure how/where to add this class, thus I just created a new file.
If you could give me feedback whether this functionality might be interesting to add/what to change I would gladly try my best to fulfill your requests.

I have not yet written any tests, apart from manually comparing outputs of qiskit and my algorithm, and simulating both circuits with qiskit. (can be added in the future)

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.

@burgholzer
Copy link
Member

Hey 👋🏼
Many thanks for the initial PR. This is definitely interesting and in scope for mqt-core! Looking forward to having this included.

The best place to put this would probably be in the algorithms submodule of mqt-core (https://github.com/cda-tum/mqt-core/tree/main/include/mqt-core/algorithms).
You can take lots of inspiration from the existing classes there. (https://github.com/cda-tum/mqt-core/blob/main/include/mqt-core/algorithms/Entanglement.hpp as one of the simpler ones, https://github.com/cda-tum/mqt-core/blob/main/include/mqt-core/algorithms/QPE.hpp as one of the more complicated examples).
The goal there would be to just create a new subclass of QuantumComputation called StatePreparation that takes the appropriate input in its constructor and then constructs the respective circuit using your methods. That shouldn't take too much adaptation from your existing code.

I would advise you to also create a .cpp file in the src/algorithms directory and move all the definitions from the header file there (just keeping the declarations themselves in the header). This makes compilation faster and helps to separate the interface of your methods from the implementation itself.
If any method is only ever used in one of your functions internally, you might as well move the declaration to the cpp file so the method is not exposed publicly. (This probably applies to most of the matrix manipulation stuff).

After the initial setup, it would be great to add some tests for the new functionality. These should probably placed here: https://github.com/cda-tum/mqt-core/tree/main/test/algorithms. Again, you can take lots of inspiration from the existing tests and should be able to use our own simulator for some experiments.

Please don't hesitate to ask questions if anything pops up!

@burgholzer burgholzer added feature New feature or request Core Anything related to the Core library and IR c++ Anything related to C++ code labels Jan 31, 2024
@burgholzer
Copy link
Member

Closing for now. Please feal free to reopen if you plan to continue working on this.

@burgholzer burgholzer closed this Aug 6, 2024
Copy link

codecov bot commented Nov 4, 2024

Codecov Report

Attention: Patch coverage is 99.31034% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.4%. Comparing base (eaedadc) to head (fad2be8).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/algorithms/StatePreparation.cpp 99.2% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff           @@
##            main    #543    +/-   ##
======================================
  Coverage   92.4%   92.4%            
======================================
  Files        126     127     +1     
  Lines      13257   13402   +145     
  Branches    2033    2060    +27     
======================================
+ Hits       12252   12391   +139     
- Misses      1005    1011     +6     
Flag Coverage Δ
cpp 92.1% <99.3%> (+<0.1%) ⬆️
python 99.7% <ø> (ø)
Files with missing lines Coverage Δ
include/mqt-core/dd/Simulation.hpp 100.0% <100.0%> (ø)
src/algorithms/StatePreparation.cpp 99.2% <99.2%> (ø)

... and 2 files with indirect coverage changes

@M-J-Hochreiter
Copy link
Contributor Author

Hi,

sorry for the long wait, I finally got around to working on this again!

I do have a question on the new constructor that should be implemented:
As my previous methods created a QuantumComputation from scratch, do you believe it makes sense to just call the QuantumComputing constructor in my StatePreparation constructor with the QuantumComputation I obtained from my prepareState function?

I do not see any option to reopen this pull request and continue working on it.

@burgholzer burgholzer reopened this Jan 13, 2025
@burgholzer
Copy link
Member

Hi,

sorry for the long wait, I finally got around to working on this again!

I do have a question on the new constructor that should be implemented: As my previous methods created a QuantumComputation from scratch, do you believe it makes sense to just call the QuantumComputing constructor in my StatePreparation constructor with the QuantumComputation I obtained from my prepareState function?

I do not see any option to reopen this pull request and continue working on it.

Hey 👋🏼

No worries. I just reopened the PR for you.
Have a look at the new versions of the algorithm implementations in MQT Core, e.g.,

I'd imagine that your State Preparation functionality code can be implemented and integrated very similarly.

@M-J-Hochreiter
Copy link
Contributor Author

I refactored both files to match the other algorithms style if you would like to give it a look!

Next step would then be tests from what I can see right?

@burgholzer
Copy link
Member

I refactored both files to match the other algorithms style if you would like to give it a look!

Next step would then be tests from what I can see right?

Getting the CI to a state where it is all-green would be desirable.
That includes writing tests that cover all the changes. 👍🏼

@burgholzer
Copy link
Member

Alright. Glad that this could be resolved 👍🏼

For some reason I currently cannot push to your fork anymore. However, below is a patch for resolving the linking errors

Index: src/algorithms/CMakeLists.txt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/algorithms/CMakeLists.txt b/src/algorithms/CMakeLists.txt
--- a/src/algorithms/CMakeLists.txt	(revision d521ec6f4f6a251ad5591894628a5a10af1b8a57)
+++ b/src/algorithms/CMakeLists.txt	(revision 948ec2b18cb510a987a4aa09fb1024efb65791c0)
@@ -22,7 +22,7 @@
   target_link_libraries(
     ${MQT_CORE_TARGET_NAME}-algorithms
     PUBLIC MQT::CoreIR
-    PRIVATE MQT::ProjectOptions MQT::ProjectWarnings)
+    PRIVATE MQT::CoreCircuitOptimizer MQT::ProjectOptions MQT::ProjectWarnings)
 
   # set include directories
   target_include_directories(

I'll quickly go through the open questions and subsequently will review the PR.
Thanks for all your work on this!

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.

Alright, many thanks for all your work on this.
This is as close to ready as it gets and turned out nicely.
I have a couple of final suggestions left, which you will find in the respective inline comments. After addressing those, this is ready to go in 🚀

Side note: while reading through the code I couldn't help but notice that it would be very interested to realise the underlying algorithm using decision diagrams as a data structure, which should simplify some of the code and also improve the general performance. Might be a nice follow up project 😌

@M-J-Hochreiter
Copy link
Contributor Author

Implemented all the requested changes. Looks done to me 😄

@M-J-Hochreiter M-J-Hochreiter marked this pull request as ready for review February 19, 2025 10:17
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.

Everything looks good to me now. 🙌🏼
Many thanks for all your work on this. 🙏🏼
The contribution turned out quite nicely! ✨

I'll apply one last small suggestion and then this can go in 🚀

@burgholzer burgholzer changed the title State Preparation in MQT ✨ ✨ Add State Preparation Algorithm Feb 21, 2025
@burgholzer burgholzer enabled auto-merge (squash) February 21, 2025 20:13
@burgholzer burgholzer added this to the MQT Core milestone Feb 21, 2025
@burgholzer burgholzer merged commit 16e022f into cda-tum:main Feb 21, 2025
31 checks passed
@M-J-Hochreiter
Copy link
Contributor Author

Thank you for all your valuable Feedback on this!
Glad that even with a bit of delay everything worked out 😄

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 Core Anything related to the Core library and IR feature New feature or request
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants