-
Notifications
You must be signed in to change notification settings - Fork 603
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
[BUGFIX] Fix qml.lie_closure
by using SVD based linear independence check
#6232
Conversation
@dwierichs @vincentmr tagged you for review, but no rush on this. I did this change on my local research branch and found it to work quite well. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6232 +/- ##
==========================================
- Coverage 99.71% 99.70% -0.01%
==========================================
Files 444 444
Lines 42234 42235 +1
==========================================
- Hits 42114 42112 -2
- Misses 120 123 +3 ☔ View full report in Codecov by Sentry. |
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.
We now have
eps = 1e-3
g = qml.lie_closure([X(0), X(0) + eps * Y(0)])
>> [X(0), X(0) + 1e-03 * Y(0), 1e-03 * Z(0)]
but
eps = 1e-9
g = qml.lie_closure([X(0), X(0) + eps * Y(0)])
>> [X(0), X(0) + 1e-09 * Y(0)]
This is because the commutator is nullified by com.simplify()
in lie_closure
. Should we pass tol
along to obtain a more consistent or predictable behaviour?
Similarly,
eps = 1e-5
g = qml.lie_closure([X(0), X(0) + eps * Y(0)], tol=1e-2)
>> [X(0)]
but
eps = 1e-5
g = qml.lie_closure([X(0), X(0) + eps * Y(0)], tol=1e-3)
>> [X(0), X(0) + 1e-05 * Y(0), 1e-05 * Z(0)]
which looks unintuitive to me.
I agree that we should do this: If a user wants to take care of tolerances themselves, they should be able to do so consistently
For default tolerances, I think we can give our best to create a consistent default, but if there are hickups in edge cases, I'd personally say that's fine :) |
You're right, I reinstalled PennyLane in a fresh |
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.
Nice upgrade @Qottmann!
I had a few small comments to be addressed :)
Thanks for the great feedback @vincentmr and @dwierichs ! |
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.
LGTM! Nice upgrade @Qottmann 💯
This fix was unblocking some research I was doing today, might be nice to get in soon :) |
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.
LGTM! Just a small suggestion for the changelog : )
Co-authored-by: Pietropaolo Frisoni <pietropaolo.frisoni@xanadu.ai>
fixes #6065
Additionally introduces normalization of vectors to avoid exploding coefficients
Could alternatively also introduce orthonormalization with
orthonormal_basis = U[:, :rank]
in the SVD step, but I dont think this is necessary.[sc-70526]