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

pinv2 deprecated #197

Open dannyrichy opened 1 year ago

dannyrichy commented 1 year ago

Hello, Since scipy 1.9.0 , the function pinv2 has been subsumed in pinv, is it possible to remove it's use in generic.py

P.S Attaching link to the release notes https://docs.scipy.org/doc/scipy/release.1.9.0.html?highlight=pinv2

FanwangM commented 1 year ago

Hi! Thank you for your interest in our package. @the-nihilist-ninja If you install the latest version of our package from the source, pinv2 has been replaced by pinv.

I am going to close this issue for now. But please feel free to reopen this issue or create a new one if needed.

lgrcia commented 1 year ago

Thanks for the tips. Would that be possible to update it on pypi?

PaulWAyers commented 1 year ago

Updating on pypi should be possible with cardboardlint.

PaulWAyers commented 1 year ago

We are again having issues here. I think we need to remove the pinv2 functionality.

The issue with Moore-Penrose pseudoinverses not working still exists, however, although it rarely occurs. (It is a shame that scipy and numpy do not give more transparent method control in linear algebra routines.) A hack that works is to add a little bit of random noise when the matrix is ill conditioned. Something like the following should work in almost all cases.

try:
    Minv = np.linalg.pinv(M)
except np.linalg.LinAlgError:
     M = 2e-14*np.random.random_sample((t1.shape[0],t1.shape[1])) - 1e-14
    M= np.linalg.pinv(M)
FanwangM commented 1 year ago

Do you mean getting an error when using pinv2? If so, I would assume that people installed our package from PyPI instead of the latest codes in the master branch. The pinv2 function has been replaced with pinv function already. If not, the way you proposed makes sense and I can make a change soon. @PaulWAyers

PaulWAyers commented 1 year ago

Yep, we can resolve this by requiring old versions of scipy, which is fine. But probably better to evolve to the newest versions of scipy anyway.

FanwangM commented 1 year ago

Can you (or who encountered the problem) provide an example showing the problem of using pinv function? I will add this as a test in the fix. Thanks.

@PaulWAyers

PaulWAyers commented 1 year ago

The issue showed up in gopt, but I remembered that the same issue appears here. @Moham21 should have an example of a matrix that's problematic.

PaulWAyers commented 1 year ago

@moham21 can you give an example of a matrix whre scipy.pinv doesn't work but scipy.pinv2 does work?

Moham21 commented 1 year ago

@Moham21 can you give an example of a matrix whre scipy.pinv doesn't work but scipy.pinv2 does work?

I checked the test errors that I had so far and I didn't come across an inverse problem, but rather an eigenvalue conversion. And we have decided to try numpy and except scipy and that helped for the most part.

FanwangM commented 1 year ago

Do you still have a matrix example to show how the pinv2 failed? This would be useful for me to build a test. Thank you. @Moham21

PaulWAyers commented 1 year ago

I talked to @Moham21 and the examples she had were from the eigenvalue problem. She was constructing a generalized inverse, but doing it "by hand" using the eigenvalues.

As I recall, the examples that failed where ones where the matrix had several rows and/or columns of zeros. I think a nilpotent matrix might be a good example, but I did not confirm this.