-
Notifications
You must be signed in to change notification settings - Fork 30
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
Improved support of parametrized gates in DDSIM Backends #293
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #293 +/- ##
=====================================
Coverage 92.5% 92.5%
=====================================
Files 32 32
Lines 2537 2553 +16
Branches 351 351
=====================================
+ Hits 2348 2363 +15
- Misses 189 190 +1
*This pull request uses carry forward flags. Click here to find out more.
|
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.
Many thanks for the PR 👍🏼
I just went through it real quick and you can find some comments below.
Most importantly, I believe that some of the code duplication can be avoided.
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.
Hey 👋🏼
Just went over the changes once more. We are definitely converging.
I like that this is a way smaller change now than at the start of the PR.
You can find a couple of comments (mostly regarding typing) inline.
Additionally, the adjustments to the tests from the previous review are still open.
I also added you as a contributor to the repository, so you should now have the permissions to (re-)request reviews.
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 again, looking better now 👍🏼
Find some further comments inline.
Modified error message Co-authored-by: Lukas Burgholzer <[email protected]>
Modified error message Co-authored-by: Lukas Burgholzer <[email protected]>
Co-authored-by: Lukas Burgholzer <[email protected]>
Signed-off-by: burgholzer <[email protected]>
use `assign_parameters` instead of the soon-to-be-deprecated `bind_parameters`. Simplify input validation since many checks are already covered by the `assign_parameters` method in strict mode. Signed-off-by: burgholzer <[email protected]>
Signed-off-by: burgholzer <[email protected]>
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 the changes.
I took the liberty to fix the remaining errors and refactored parts of the code to be even simpler.
Most notably:
bind_parameters
has been replaced byassign_aparameters
since the former function is soon to be deprecated (see Deprecatebind_parameters
in favor ofassign_parameters
Qiskit/qiskit#10792)- Several checks have been simplified as they are partly covered by the
assign_parameters
method internally or could be simplified by swapping some statements around - Typing of the parameters was simplified by introducing a type alias and using the more generic version from Qiskit
From my end, this looks good to go now. Let me know if you see anything problematic.
This PR expands the handling of parameterized circuits by DDSIM backends. DDSIM backends can now bind parameters to quantum circuits, resembling the way Qiskit Primitives operate.
Solves #295 first task.