-
Notifications
You must be signed in to change notification settings - Fork 363
Implemented Cholesky decomposition to numpy and tensorflow backends #890
Conversation
...kends, fixing various pylint errors and adding test to each respective backend function
LuiGiovanni
commented
Dec 25, 2020
@alewis @mganahl Hello! I implemented the Cholesky decomposition to various backends following the idea of a pivot value for the tensors, with their respective tests where I defined a matrix that is assured to be positive-definite and also a hermitian matrix:
[1, 0]
[0, 1]
I assumed we won't be validating if the matrices are hermitian or positive-definite since there could be some performance cost in doing so (at least for positive-definite checking) the addition of a hermitian check would be nice to have.
A Pylint error has been occurring in the test runs, but it is somewhere in the code where I haven't made changes to. the file in question is tensornetwork/tn_keras/conv2d_mpo.py which seems to only be affected in versions 3.6 and 3.7, this issue is also occurring in my other PR #889 maybe freezing the version could fix this but this is a lazy solution and would not solve it for all cases, so I will wait for your feedback to see what can be done here. Here is an image of the error for reference.
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.
Rename tf to np (the user passes the numpy module here)
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.
ya
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.
Using the identity matrix as "random" matrix is not an ideal test case. You can generate a positive definite matrix e.g. by
A = np.random.rand(D,D) A = A @ A.T.conj()
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.
btw. also test this for all supported dtypes
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.
do np.random.rand(D, D).astype(dtype) to get your fav type. if it's imaginary you might need to initialize the real and imaginary parts separately.
put np.random_seed(10) (or any other fixed integer) before the rand so that the matrix is fixed.
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.
There is no dtype defined in any of the tests, should I define one myself?
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 is unnecessary. The cholesky decomposition should already return a lower-triangular matrix with a real, positive diagonal.
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.
remove non_negative argument
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 only makes sense for QR)
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.
remove non_negative_diagonal (see above)
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.
use better test matrix (see above)
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.
see above
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.
see above
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.
see above
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.
modify test, see above
mganahl
commented
Dec 28, 2020
A Pylint error has been occurring in the test runs, but it is somewhere in the code where I haven't made changes to. the file in question is
tensornetwork/tn_keras/conv2d_mpo.pywhich seems to only be affected in versions 3.6 and 3.7, this issue is also occurring in my other PR #889 maybe freezing the version could fix this but this is a lazy solution and would not solve it for all cases, so I will wait for your feedback to see what can be done here. Here is an image of the error for reference.
Thanks for pointing this out. Likely this is due to a new tensorflow release. I'll fix it.
Codecov Report
@@ Coverage Diff @@ ## master #890 +/- ## ======================================= Coverage 98.00% 98.00% ======================================= Files 129 129 Lines 22625 22665 +40 ======================================= + Hits 22173 22213 +40 Misses 452 452
Continue to review full report at Codecov.
|
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.
ya
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 only makes sense for QR)
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.
do np.random.rand(D, D).astype(dtype) to get your fav type. if it's imaginary you might need to initialize the real and imaginary parts separately.
put np.random_seed(10) (or any other fixed integer) before the rand so that the matrix is fixed.
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.
add a newline here (at the end of the file)
...i/TensorNetwork into CholeskyDecomposition
LuiGiovanni
commented
Dec 28, 2020
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.
you are passing the tensorflow module here, but it should be numpy
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.
np is the numpy module. numpy does not have reduce_prod, that's a tensorflow routine. This only passes the test because in decompositions_test.py you are erroneously passing the tensorflow module instead of the numpy module to numpy.decompositions.cholesky. pls fix 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.
you have to call the torch version of choleksy here, not numpy.
mganahl
commented
Jan 18, 2021
Hi @LuiGiovanni, once the comments are fixed we can merge this
Added the decomposition function following the idea of having a pivot on the tensor, this is in reference to issue #852. which we then reshape into a matrix for the Cholesky function used in NumPy linear algebra. which you may find here for reference.
I Will start working on adding the function to the missing backend which is symmetric since Jax uses NumPy's decomposition file. I await any feedback you may have and that you for the help!