-
Notifications
You must be signed in to change notification settings - Fork 20
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
Test error in "SVD Converge" in generic.py #69
Comments
It may still have a problem, I guess, because matrices with rows/columns of zeros are evil..... It's a strange error (that we encountered once before in |
These tests passed on Ubunto with python 3.8 before. Is GitHub Action using two different hardware for these runs? The new runs failed again: https://github.com/theochem/procrustes/runs/2069045083#step:5:354 |
The solution here, which may also be useful to @tczorro , is to use LAPACK directly. Construct the factorized SVD (more robust than the divide-and-conquer iterative algorithm), using (I think) scipy.linalg.lapack.gesvd though you may need to prefix the gesvd routine with dgesvd (float) or zgesvd (complex). Once you have the SVD (and this one should always work), you can then use
The traditional threshhold value for step 1 is, for an mxn input matrix A, max(m,n)*epsilon(elements of A). I.e., it is the machine-precision for the elements of A times the number of elements in the larger dimension. This could be approximated easily by merely using max(m,n)*1e-15, which is good enough. This can be made (potentially much) more efficient by truncating the singular-value-inverse, U, and V to include only the elements corresponding to the nonzero rows/columns. That way the matrix multiplication is faster (because you are not computing a lot of "multiply something by zero" terms). |
We can use the |
The generic function uses Morse Penrose Inverse from numpy and it uses the divide and conquer method from Lapack (gesdd) to calculate the SVD, whereas SciPy's Morse Penrose inverse uses the general rectangular approach ('gesvd') [1]. So I think that we just need to change from numpy to scipy. |
That's a good trick. @tczorro should look at it too. You have to fall down the rabbit-hole of documentation a bit to find these things sometimes :-( . If this works, it may be worth remembering that if someone tried a VERY huge matrix, the speed/memory benefits of the pinv2 algorithm in scipy/numpy might help. But Numpy had a stupid tolerance. And if you have a huge matrix, the robustness of a "real SVD" instead of an "iterative SVD" is liekly to be important..... |
I think |
I would use scipy.linalg.svd with lapack_drive='gesvd' everywhere. Where that is not appropriate (where speed is critical) probably there are better choices than SVD (which is a bit of a brute-force approach inherently). |
Using Thanks for proposing to use |
@Ali-Tehrani can you please take care of using |
Related to issue theochem#69, since scipy svd allows better control of the type of SVD algorithm that is being used.
Related to issue theochem#69, added to rotation, permutation and symmetric.
During testing with github action, the follow test failed "test_generic_rectangular_translate_scale" for test_generic.py" with m = 829, n = 878 on python 3.7.
The error is
def _raise_linalgerror_svd_nonconvergence(err, flag): raise LinAlgError("SVD did not converge") E numpy.linalg.LinAlgError: SVD did not converge
.For more info on the github action, see https://github.com/Ali-Tehrani/procrustes/runs/2058961187.
However, running this test on my own machine it passes. So it seems the error is hardware related. Google searching this returns a 2019 SciPy issue [1]. I would recommend to decrease the matrix size so that it isn't hardware/MKL/OpenBlas dependent.
[1] - scipy/scipy#10032
The text was updated successfully, but these errors were encountered: