-
Notifications
You must be signed in to change notification settings - Fork 606
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
angle solver qsp/qsvt #6483
base: master
Are you sure you want to change the base?
angle solver qsp/qsvt #6483
Conversation
Hello. You may have forgotten to update the changelog!
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6483 +/- ##
=======================================
Coverage 99.46% 99.46%
=======================================
Files 454 454
Lines 42649 42716 +67
=======================================
+ Hits 42420 42487 +67
Misses 229 229 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Co-authored-by: Yushao Chen (Jerry) <[email protected]>
@@ -123,6 +124,7 @@ def qsvt(A, angles, wires, convention=None): | |||
A = qml.math.reshape(A, [1, 1]) | |||
|
|||
c, r = qml.math.shape(A) | |||
global_phase, global_phase_op = None, None |
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.
I had to put this in because codefactor complained that I was using global_phase before giving it any value in line 148
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.
Mostly small comments on the doc-strings and general questions. Great work! The PR looks good.
poly_degree = len(P) - 1 | ||
|
||
# Build the polynomial R(z) = z^degree * (1 - conj(P(1/z)) * P(z)), deduced from (eq.33) and (eq.34) | ||
R = Polynomial.basis(poly_degree) - Polynomial(P) * Polynomial(np.conj(P[::-1])) |
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.
It doesn't seem like it matches the comment?
R = Polynomial.basis(poly_degree) - Polynomial(P) * Polynomial(np.conj(P[::-1])) | |
R = Polynomial.basis(poly_degree) * (1 - Polynomial(P) * Polynomial(np.conj(P[::-1]))) |
Also why do you need to do P[::-1]?
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.
Yep, actually this is a bit unintuitive at first glance
The key here is that if you have
Which is equal to
Which is equal to
and it can be simplified as the expression showed
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.
Basically, here we decomposed the reflection into reversal + trasnlation
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.
Please consider adding a brief description as comment to the code, specially if you have modified the equations of the paper.
([-0.4, 2, 0.4, 0, -0.1, 0, 0.1]), | ||
], | ||
) | ||
def test_transform_angles(self, angles): |
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.
This just tests that transform(transform(angles, "QSP", "QSVT"), "QSVT", "QSP") is the identity operation. We should check that for a particular polynomial's QSP angles should produce the same poly with transformed QSVT angles.
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.
internally when I ask for the QSVT angles, I calculate the QSP angles and apply the function. So it is going always fulfilled (whether the function works correctly or not).
Co-authored-by: Jay Soni <[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 @KetpuntoG, left my first round of comments.
|
||
for x in [-1, 0, 1]: | ||
if qml.math.abs(qml.math.sum(coeff * x**i for i, coeff in enumerate(poly))) > 1: | ||
# It is not a property that we can check globally but checking these three points is useful |
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.
Not sure I understand this.
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.
P should mee that |P(x)| ≤ 1
As it is not always easy to detect and the user may not be aware, I am checking that it is true at least in x = -1, x = 0, x = 1. I updated the comment 👍
[skip ci] Co-authored-by: soranjh <[email protected]>
Context:
This PR appears in the context of the QSVT project, specifically focusing on the implementation of the angle solver, functionality to calculate the angles needed to apply a polynomial transformation.
Description of the Change:
Added
qml.poly_to_angles
, the main functionality of the PR that takes as input a polynomial and returns the angles.Also included is the function
qml.transform_angles
, which transforms the angle format between qsp and qsvt.Both these functions and other auxiliary functions have been added in
qsvt.py
.Benefits:
Calculating angles is not a simple task and existing libraries have some limitations. With this new functionality we will facilitate the use of routines such as qsp and qsvt.
Possible Drawbacks:
Currently only one solver has been introduced. In the future it is expected to add more that will work for more extreme cases: larger polynomials (degree > 1000) or degenerate polynomials
[sc-72631]