-
-
Notifications
You must be signed in to change notification settings - Fork 6
Self-implemented QUBO and QAOA for the SatelliteSolver #152
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
Conversation
…s' by implementing QAOA to solve a QUBO problem. Additional fixes to support Qiskit >= 2.0.0.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #152 +/- ##
=======================================
+ Coverage 80.5% 84.0% +3.4%
=======================================
Files 8 8
Lines 875 883 +8
=======================================
+ Hits 705 742 +37
+ Misses 170 141 -29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
burgholzer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @tobi-forster for getting this effort started. 🙌
I just glanced over the PR here and left some feedback inline. This still needs quite a bit of work to be ready to merge. But should be totally doable.
Just as a mental note for the future: this report needs a CI/CD and linting setup that is closer to our main repositories. Some of the comments would have been caught automatically by that already.
burgholzer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your iteration on the code.
I did not check every single line of code here anymore but still made it through the entire PR.
You'll find a couple of technical comments inline.
None of them should be too hard to address.
In the future, if a review includes explicit suggestions for changes, it would be beneficial to actively utilize them. GitHub lets you directly apply them (which also has the nice advantage of resolving the conversation). For that, you can simply go to the "Files changed" tab, and you will see buttons with "Add suggestion to batch" right at the bottom of the comments containing suggestions. Simply add all comments you want to incorporate to a batch and then hit the "Commit" suggestions button on the top of the site.
It's just way more convenient for everyone involved.
Additionally, I want to express a general observation: Unfortunately, I have very low confidence that the code will work as intended, either because of the lack of tests or because of how the tests are written. Most of the tests are merely running some functions and not asserting anything about the correctness of the results.
I suppose there is nothing to change in this PR anymore. Let's just try to address the open comments and get it merged. But when further refactoring and developing this library, let's please try to keep this in mind.
Co-authored-by: Lukas Burgholzer <burgholzer@me.com> Signed-off-by: Tobias Forster <158472669+tobi-forster@users.noreply.github.com>
# Conflicts: # .pre-commit-config.yaml
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
burgholzer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright. I pushed quite a couple of updates to this PR. Almost none actually change the code. Most of them revolve around proper packaging, dependency setup, and CI/CD.
This also brings the problem solver closer to the MQT best practices.
Most importantly, this now also includes CI runs with the minimum versions specified for all dependencies. I did not notice in my previous review that these weren't even set up here.
I also fixed a couple of new deprecation warnings raised by Qiskit.
I briefly tried to get this working with qiskit < 2, but gave up with the tweedledum dependency after a couple of minutes. Let's stick with Qiskit 2.0+ for now.
If CI passes here, this is ready to be merged.
|
@tobi-forster Tests here are sporadically failing because of Can you please tweak the tests so that they are less flaky and prone to fail? |
burgholzer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍🏼 let's get this in.
This PR marks the first step of updating the MQT ProblemSolver to support Qiskit >= 2.0.0. For this, the dependencies 'qiskit_optimization' and 'qiskit_algorithms' have to be removed. To this end, the QUBO to QAOA process in the SatelliteSolver has been implemented manually. Additionally, small fixes have been made to support Qiskit >= 2.0.0.