theochem / procrustes

Python library for finding the optimal transformation(s) that makes two matrices as close as possible to each other.
https://procrustes.qcdevs.org/
GNU General Public License v3.0
109 stars 20 forks source link

Deprecated `pinv2` function in `scipy.linalg` #174

Closed FanwangM closed 2 years ago

FanwangM commented 2 years ago

The pinv2 in Scipy has been deprecated since 1.7.0, and it has been removed for 1.9.0 and above, https://github.com/scipy/scipy/pull/16245. Originally, the (Moore-Penrose) pseudo-inverse is computed by SVD including all 'large' singular values (using scipy.linalg.pinv2) and scipy.linalg.pinv obtains the (Moore-Penrose) pseudo-inverse by using least-squares solver. The least-squares implementation is less efficient, but more robust, than the SVD implementation (https://github.com/theochem/procrustes/blob/5c58d97735bfe2ec298a868f35d501f943e1e41b/procrustes/generic.py#L93-L98). But later, I think both functions use the SVD solution. Therefore, it was proposed to remove pinv2.

So, for newer versions of Scipy >= 1.9.0, it would be a problem to use pinv2. Therefore, I think the easiest solution is to stick to pinv and remove pinv2. Another option is to provide different solutions for different versions of Scipy greater than 1.9.0 or less than 1.9.0. What do you prefer? @PaulWAyers @Ali-Tehrani

PaulWAyers commented 2 years ago

I think there is no choice here. It bothers me that under-the-hood the SVD is the iterative SVD that can fail due to "lack of convergence" and this is a problem we've observed happens sometimes, as opposed to the rigorous factorization of a matrix which is always fine. But with current versions, scipy.pinv, scipy.pinv2, and numpy.pinv are all the same. No need to distinguish.

See the discussion https://github.com/scipy/scipy/issues/8861#issuecomment-1030803006

PaulWAyers commented 2 years ago

We need to remember this: if we end up with "SVD did not converge" errors we will need to add in a brute strength implementation using the dgesvd SVD driver (accessible in scipy).