TensorNetwork icon indicating copy to clipboard operation
TensorNetwork copied to clipboard

Implemented Cholesky decomposition to numpy and tensorflow backends

Open LuiGiovanni opened this issue 5 years ago • 5 comments

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!

LuiGiovanni avatar Dec 23 '20 21:12 LuiGiovanni

@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.

image

LuiGiovanni avatar Dec 25 '20 20:12 LuiGiovanni

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.

image

Thanks for pointing this out. Likely this is due to a new tensorflow release. I'll fix it.

mganahl avatar Dec 28 '20 10:12 mganahl

Codecov Report

Merging #890 (92c931a) into master (f669725) will increase coverage by 0.00%. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #890   +/-   ##
=======================================
  Coverage   98.00%   98.00%           
=======================================
  Files         129      129           
  Lines       22625    22665   +40     
=======================================
+ Hits        22173    22213   +40     
  Misses        452      452           
Impacted Files Coverage Δ
tensornetwork/backends/numpy/decompositions.py 100.00% <100.00%> (ø)
...ensornetwork/backends/numpy/decompositions_test.py 98.75% <100.00%> (+0.11%) :arrow_up:
tensornetwork/backends/pytorch/decompositions.py 100.00% <100.00%> (ø)
...sornetwork/backends/pytorch/decompositions_test.py 100.00% <100.00%> (ø)
...ensornetwork/backends/tensorflow/decompositions.py 100.00% <100.00%> (ø)
...network/backends/tensorflow/decompositions_test.py 98.96% <100.00%> (+0.08%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f669725...92c931a. Read the comment docs.

codecov-io avatar Dec 28 '20 11:12 codecov-io

Hi there @mganahl @alewis In the end I implemented the test following your requested changes and tried these changes out, I'd like your opinion on them. The biggest difference being:

image

torch.from_numpy(random_ matrix) since it wasn't reading it like a tensor. Let me know what you think. Thank you!

LuiGiovanni avatar Dec 28 '20 21:12 LuiGiovanni

Hi @LuiGiovanni, once the comments are fixed we can merge this

mganahl avatar Jan 18 '21 10:01 mganahl