-
Notifications
You must be signed in to change notification settings - Fork 604
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
[labs.dla 3] structure_constants_dense #6376
base: dense_util
Are you sure you want to change the base?
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 @@
## dense_util #6376 +/- ##
===========================================
Coverage 99.29% 99.29%
===========================================
Files 454 454
Lines 43262 43267 +5
===========================================
+ Hits 42959 42964 +5
Misses 303 303 ☔ View full report in Codecov by Sentry. |
…into structure_constants_dense
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 to see this polished up so neatly after using it internally already :D 💯
I again only had small comments, and one request for another test case.
@pytest.mark.parametrize("dla", [Ising3, XXZ3]) | ||
@pytest.mark.parametrize("use_orthonormal", [False, True]) | ||
def test_structure_constants_elements(self, dla, use_orthonormal): | ||
r"""Test relation :math:`[i G_\alpha, i G_\beta] = \sum_{\gamma = 0}^{\mathfrak{d}-1} f^\gamma_{\alpha, \beta} iG_\gamma`.""" | ||
|
||
d = len(dla) | ||
dla_dense = np.array([qml.matrix(op, wire_order=range(3)) for op in dla]) | ||
if use_orthonormal: | ||
gram_inv = sp.linalg.sqrtm( | ||
np.linalg.pinv(np.tensordot(dla_dense, dla_dense, axes=[[1, 2], [2, 1]]).real) | ||
) | ||
dla_dense = np.tensordot(gram_inv, dla_dense, axes=1) | ||
dla = [(scale * op).pauli_rep for scale, op in zip(np.diag(gram_inv), dla)] | ||
ad_rep = structure_constants_dense(dla_dense, is_orthonormal=use_orthonormal) | ||
for i in range(d): | ||
for j in range(d): | ||
|
||
comm_res = 1j * dla[i].commutator(dla[j]) | ||
res = sum((c + 0j) * dla[gamma] for gamma, c in enumerate(ad_rep[:, i, j])) | ||
res.simplify() | ||
assert set(comm_res) == set(res) # Assert equal keys | ||
if len(comm_res) > 0: | ||
assert np.allclose(*zip(*[(comm_res[key], res[key]) for key in res.keys()])) |
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 think the tests are nice. We should add one on a non-orthonormal set of operators, though, that checks exactly this defining property of the structure constants :)
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.
(Can do this just by applying some random coefficients to the existing DLAs.)
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 guess to be truly non-orthonormal we should also break the orthogonality, so doing some sums of paulis
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.
Ah yeah, I meant a non-diagonal coefficients matrix :)
Like
coeffs = np.random.random((len(dla), len(dla)))
new_dla = [qml.sum(*(c * op for c, op in zip(_coeffs, dla))) for _coeffs in coeffs]
or so
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 added a tested orthonormalize function but somehow the test now fails for the cases where I orthonormalize the basis 🤔 I am confused
edit: talking about the test_structure_constants_elements
test, the tests for orthonormalize
work as expected
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 think the problem is the inconsistency between the trace inner product for matrices and pauli sentences that is assumed. I.e. the pauli sentence one is normalized whereas in the matrix case we dont. So I think to have consistent adjoint representations we should fix one convention and stick to that. Not sure which one though?
x = X(0) @ X(1)
x = x.pauli_rep
(x @ x).trace()
# 1.0
xm = qml.matrix(x, wire_order=range(2))
np.trace(xm @ xm)
# 4.0
I feel like the normalized one is neater because it should not depend on the (potentially redundant) matrix dimensions. E.g. in the example if you set the wire_order to range(3) the inner product is 8
In particular, I would suggest the matrix inner product to be
xm = qml.matrix(x, wire_order=range(4))
np.trace(xm @ xm)/len(xm)
to match those pauli sentences.
I pushed some suggestions but the sructure_constants test is still failing when the operators are being orthonormalized 🤔
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.
Fully agree that we should be using
…into structure_constants_dense
Co-authored-by: David Wierichs <[email protected]>
…aneAI/pennylane into structure_constants_dense
Building on top of #6371
An implementation of
structure_constants
using dense matrices, which sometimes is advantageous in certain scenarios.The projection routine can be optimized still
[sc-75525]