Thanks @aarmey! I like the idea of the refactoring - SVD_FUNS was a hacky solution to a problem that appeared when introducing the backend system. So, how about adding a parameter (e.g. svd_fun) to truncated_svd and making that common to all implementations, which will then just call the various svd funs to actually perform the computation? We can pass an additional parameter to svd_fun, n_eigenvecs for instance, which can be ignored when not used (scipy's sparse eigendecomposition uses it efficiently while standard svd does not in general).

Regarding mxnet.np, indeed, I was waiting for it to be finalized to switched, we can open a separate PR for that!

Regarding kr, this implementation was much faster than other ones when I wrote it. Since we now have various tenalg backends, one core one using "regular" methods (e.g. reshapings + broadcasting and matmuls) and an einsum one that dispatches all operations to einsum, so we shouldn't force one or the other directly. Instead, all functions where we have a choice, such as khatri-rao, can be put in tensorly.tenalg (both einsum_tenalg and core_tenalg). In this case, Khatri-Rao is defined for both.

As a side note, I would like to enable dispatching tenalg calls directly to the backend in case they implement a particular function but I'm worried it can become a big mess, it's already not the most intuitive :/

Codecov Report

Merging #217 (b0503cb) into main (dc419c5) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #217      +/-   ##
==========================================
+ Coverage   87.00%   87.02%   +0.02%     
==========================================
  Files          82       86       +4     
  Lines        3825     4055     +230     
==========================================
+ Hits         3328     3529     +201     
- Misses        497      526      +29     

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 dc419c5...546f2f3. Read the comment docs.

Thanks a lot @aarmey, this is a large but necessary change to make the behaviour of SVD clearer to users.

Overally looks good to me, the change from _reverse to flip is great.
I think we may want to use the opportunity to add some documentation, perhaps in the user_guide, either in the backend section or in a new page? We can there explain the difference between the three provided interfaces.

I'd really rather avoid converting to NumPy as much as possible as it breaks the computational graph in some backends and causes surprising issues (e.g. if running the method on GPU).

On the API side, I'm wondering whether we want to provide a function partial_svd (or even just svd) and an additional implementation (or method?) :

def partial_svd(matrix, n_eigenvecs, implementation='truncate')

where

implementation : {'truncate', 'numpy', 'eish'}

          - truncate : uses the backend's native SVD function and truncates it
          - numpy : uses the efficient Sparse SVD from Scipy to only compute the required eigenvalues
          - symeig : uses the native backend's symeig to leverage the symmetry in the covariance matrix when computing the SVD

What do you think?

I think this SVD interface looks great. Indeed, I partly wanted these changes to move more of the SVD methods to the proper backend as opposed to silently going back to NumPy. I like that your suggestion will make it clear whether the method is using the indicated backend or Scipy.

Perhaps I can open an issue with your notes here? I expect that changing the SVD interface is going to be another large, complicated change.

Yes, that's a good idea. There are some conversions to NumPy in the reconstruction errors, would be great to remove those before merging if possible!

@JeanKossaifi removed.