-
Notifications
You must be signed in to change notification settings - Fork 714
upgrade qml.qsvt #6520
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
upgrade qml.qsvt #6520
Conversation
…nylane into angle_solver_qsp
|
Hello. You may have forgotten to update the changelog!
|
Jaybsoni
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.
Looks good, left some suggestions. Let me know once they are addressed and I am happy to review again!
Also missing change-log entry 😉
|
Thanks for the review @Jaybsoni 🙌 |
DSGuala
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!
Left a few comments about errors I ran into and potential doc/error message improvements.
AntonNI8
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.
Looks good for the most part, just needs:
qsvt_legacy:
-deprecation warning, suggestion to use qsvt instead.
-update deprecation docs accordingly.
qsvt:
-Raise a warning when qsvt is called without the new keyword arguments.
-In the warning, state the (new) function signature, and ask users to provide poly rather than angles, and to use the keyword arguments to silence the warning.
-Refer them to qsvt_legacy for access to the previous functionality.
Jaybsoni
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.
Looks good to me, just a few minor suggestions.
Regarding Diego's comment about block-encoding, I am confused as to why it's causing issues. Let's chat offline to discuss the technical problem. Conditionally approving based on the result of that discussion.
|
Based on the last discussion with Jay and Diego:
|
DSGuala
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.
Looks nearly good to go. Approved pending a few comments 👍
Co-authored-by: Diego <67476785+DSGuala@users.noreply.github.com>
This PR upgrades
qml.qsvtbased on this PRD . To check how the documentation is displayed -> [here]The main changes are:
qml.qsvtonly worked withqml.BlockEncode. With the newqml.qsvtthe user can choose the specific block encoding.qml.qsvtthe user had to pass the appropriate angles to apply a polynomial. Now the user passes the polynomial directlyThese changes aim to make
qml.qsvt, the friendly (but less customizable) version to use QSVT.Old functionality is moved to
qml.qsvt_legacy.[sc-72651] [sc-72650]