-
Notifications
You must be signed in to change notification settings - Fork 643
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
upgrade qml.qsvt #6520
Conversation
…nylane into angle_solver_qsp
Hello. You may have forgotten to update the changelog!
|
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 🙌 |
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.
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.
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:
|
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.qsvt
based on this PRD . To check how the documentation is displayed -> [here]The main changes are:
qml.qsvt
only worked withqml.BlockEncode
. With the newqml.qsvt
the user can choose the specific block encoding.qml.qsvt
the 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]