Add TTOI functions #411
Add TTOI functions #411
Conversation
Codecov Report
@@ Coverage Diff @@
## main #411 +/- ##
==========================================
+ Coverage 86.84% 87.01% +0.17%
==========================================
Files 118 120 +2
Lines 7313 7432 +119
==========================================
+ Hits 6351 6467 +116
- Misses 962 965 +3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Thanks @Lili-Zheng-stat, it's much clearer! I left some mostly stylistic comments in the code. Would be great to apply pep8 formatting too.
Looking to have TTOI in TensorLy!
| and len(rank) == len(tl.shape(data_tensor))+1 | ||
| n_iter : int | ||
| half the number of iterations | ||
| trajectory : bool, optional, default is False |
There was a problem hiding this comment.
Is there a practical case where we'd actually want this? It would be quite memory heavy.
There was a problem hiding this comment.
This option can be set as false in practice; I am only leaving this option for sanity check in the test function, making sure that our iteration is giving better approximation and estimation when iteration number increases!
|
Thanks @Lili-Zheng-stat! The issue in the tests comes from the fact that we changed the SVD interface in #429, the best would be to use |
Good idea. I'll add a deprecation before we think about releasing this. |
Thanks for checking the errors! I updated the code so that it calls the new svd_interface now. |
|
You can format your files with Black. Then, it will pass the test. To use Black; |
|
Thanks @Lili-Zheng-stat -- sorry for the delay, I'll try to make another review asap. Would be great if someone else could also have a look, maybe @merajhashemi since you're familiar with TT too? |
There was a problem hiding this comment.
Hi @Lili-Zheng-stat, the code looks nice and the method is cool :)
I left a few comments. Maybe it could be nice to see some example of the code in action, possibly added to the Tensorly examples? But that can be done later in a subsequent PR.
| for iteration in range(n_iter): | ||
| # first perform forward update | ||
| # left_singular_vectors will be a list including estimated left singular spaces at the current iteration | ||
| left_singular_vectors = list() |
There was a problem hiding this comment.
It would be nice to provide some sort of initialization. In the paper referenced in the comments, the authors suggest to initialize with TT-SVD, which we have available in Tensorly. Maybe you could use that for initialization by default, and add an input option in the signature to allow custom initialization. You can check how we handle initialization in e.g. the _cp.py file.
There was a problem hiding this comment.
Thanks for the suggestion! However, our later updates make use of many intermediate product obtained during the TT-SVD algorithm, and it would need a lot of changes in our code structure if we want to use the default TT-SVD function.... If possible, I would prefer not changing this now...
There was a problem hiding this comment.
I meant using TT-SVD for initialization, not for the updates. But if this is a huge modification then sure the current version is fine.
Would be nice still to allow custom initialization, that does not seem too hard?
Merge the updates from tensorly since the svd_interface is in need for the current tensor-train-OI code
… instead of half the number of iterations
Lili-Zheng-stat
requested review from
JeanKossaifi and
cohenjer
and removed request for
JeanKossaifi and
cohenjer
|
It looks good to me, thanks for all the change @Lili-Zheng-stat -- it looks good to me. Can you just skip the test for MXNet (and maybe add a small test on 3rd or 4th order tensor). MXNet currently doesn't support >6th order tensors. Can you also add the new function to the API? I think we can merge it afterwards! |
|
@Lili-Zheng-stat apologies, the tests are failing due to an unrelated issue, could you merge main, that should fix the issue! Thanks! |
|
|
Yes, I realized you enabled edit from maintainers so I went ahead and merged / fixed the issue, looks like tests are passing now, merging. Thanks a lot for the PR @Lili-Zheng-stat, this is a great addition! It would be great to add an example in a separate PR. |
|
Awesome!! Thanks to everyone @JeanKossaifi @cohenjer @merajhashemi @aarmey @caglayantuna for the review and all the suggestions! |
I just created a new branch in my repository that is updated with the current version of TensorLy. The updated tensor train functions (
tt_TTOI.pyin tensorly/tensorly/contrib/decomposition, andtest_tt_TTOI.py in tensorly/tensorly/contrib/decomposition/tests) are also added and pass the tests withbackend = numpyorpytorch.