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

upgrade qml.qsvt #6520

Merged
merged 64 commits into from
Dec 6, 2024
Merged

upgrade qml.qsvt #6520

merged 64 commits into from
Dec 6, 2024

Conversation

KetpuntoG
Copy link
Contributor

@KetpuntoG KetpuntoG commented Nov 5, 2024

  • Add changelog

This PR upgrades qml.qsvt based on this PRD . To check how the documentation is displayed -> [here]

The main changes are:

  • old qml.qsvt only worked with qml.BlockEncode. With the newqml.qsvt the user can choose the specific block encoding.
  • new function also accept Hamiltonians as input instead of just matrices.
  • In old qml.qsvt the user had to pass the appropriate angles to apply a polynomial. Now the user passes the polynomial directly

These 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]

@KetpuntoG KetpuntoG added the do not merge ⚠️ Do not merge the pull request until this label is removed label Nov 5, 2024
Copy link
Contributor

github-actions bot commented Nov 5, 2024

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

Copy link
Contributor

@Jaybsoni Jaybsoni left a 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 😉

@KetpuntoG KetpuntoG requested a review from Jaybsoni November 6, 2024 15:22
@KetpuntoG
Copy link
Contributor Author

Thanks for the review @Jaybsoni 🙌
I answered your questions and included your suggestions
I will add the changelog at the end when we are clear when we are going to merge it to avoid early conflicts

Copy link
Contributor

@DSGuala DSGuala left a 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.

@KetpuntoG KetpuntoG requested a review from AntonNI8 December 4, 2024 14:46
Copy link
Contributor

@AntonNI8 AntonNI8 left a 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.

@KetpuntoG KetpuntoG removed the do not merge ⚠️ Do not merge the pull request until this label is removed label Dec 4, 2024
Copy link
Contributor

@Jaybsoni Jaybsoni left a 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.

@KetpuntoG
Copy link
Contributor Author

Based on the last discussion with Jay and Diego:

  • BlockEncode will be the new default for matrices
  • There is a more explanatory error in the cases where FABLE does not work

@KetpuntoG KetpuntoG requested a review from AntonNI8 December 6, 2024 19:34
Copy link
Contributor

@DSGuala DSGuala left a 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 👍

KetpuntoG and others added 3 commits December 6, 2024 15:36
@KetpuntoG KetpuntoG merged commit 86e4c0e into master Dec 6, 2024
46 checks passed
@KetpuntoG KetpuntoG deleted the upgrade_qsvt_2 branch December 6, 2024 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants