Skip to content
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

MAINT: symeig -- now that's a name I've not heard in a long time #3599

Merged
merged 10 commits into from
May 20, 2014

Conversation

argriffing
Copy link
Contributor

The lobpcg code has some references to symeig which has been replaced by eigh in scipy. This PR removes those old references.

The code also has some non-working benchmarks and tests which I've partially updated, but I've run into some difficulties so this part is not yet finished. One difficulty is that lobpcg is the name of a subpackage, a module, and a function, and this is causing some import weirdness. Anyone have a suggestion as to how these files or functions should be renamed?

Another difficulty is that lobpcg uses numpy arrays, matrices, and abstract linear operators, and the old non-working testing and benchmarking code seems to attempt to use an obsolete interface to lobpcg. I might try to fix this in the future. For example this old non-working code attempts to pass a plain python function that linearly maps vectors to vectors, whereas the current lobpcg interface expects something more like a LinearOperator or array or matrix to represent such a linear operator. The state of generic linear operator interfaces in numpy/scipy is somewhat dismal. For example here's yet another new github project added a couple weeks ago to try implementing such an interface.

@argriffing
Copy link
Contributor Author

temporarily closing while I move some docs...

@argriffing argriffing closed this May 1, 2014
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 951c0f6 on argriffing:remove-symeig into 7d74cc6 on scipy:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 951c0f6 on argriffing:remove-symeig into 7d74cc6 on scipy:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 1facfae on argriffing:remove-symeig into 7d74cc6 on scipy:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 1facfae on argriffing:remove-symeig into 7d74cc6 on scipy:master.

@argriffing
Copy link
Contributor Author

@rc I'm trying to bring the lobpcg code back up to date so that its tests and benchmarks will run again. Do you know much about the X.txt file and the example in the __name__ == '__main__' section of the lobpcg.py file? I'm tempted to delete them..

@argriffing
Copy link
Contributor Author

As a possible separate issue, scipy.sparse.linalg.eigsh doesn't seem to converge for the benchmark examples even for small n, whereas eigh and lobpcg both get the correct eigenvalues. I almost want to blame ARPACK (as in http://forge.scilab.org/index.php/p/arpack-ng/issues/1259/) but I'm not sure.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 13bb729 on argriffing:remove-symeig into 7d74cc6 on scipy:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 13bb729 on argriffing:remove-symeig into 7d74cc6 on scipy:master.

@pv
Copy link
Member

pv commented May 5, 2014

If you can isolate a failing eigsh case (save the matrices to files), then that can be pursued further.
However, I guess with lanczos, convergence is not a given.

@rc
Copy link
Contributor

rc commented May 5, 2014

@argriffing Yes, please, delete the 'main' section. It's probably a remnant from time when no proper tests were written.

@argriffing
Copy link
Contributor Author

@rc Thanks! Now I see what it was doing. It was checking the preconditioned generalized eigendecomposition of the diagonal matrix 1..n and where B=I (so not really generalized). The X.txt was just a random n x 3 initialization for reproducibility. I'll try to add this back as a test. For what it's worth, this is the same system used in a recent ARPACK 3.1.5 bug report http://forge.scilab.org/index.php/p/arpack-ng/issues/1397/ so eigs or eigsh or both may fail for this example because of that bug.

@pv
Copy link
Member

pv commented May 5, 2014

Potential ARPACK bug then. There's a suggestion to fix it by using LAPACK 2.0 version of DLAHQR (plus some other discussion) here: http://forge.scilab.org/index.php/p/arpack-ng/issues/1315/

If DLAHQR -> DLAHQR2 fixes it, we should just patch it ourselves, and pressure ARPACK-NG upstream to fix it in the same way.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 5756f45 on argriffing:remove-symeig into 7d74cc6 on scipy:master.

@argriffing
Copy link
Contributor Author

I agree that an ARPACK bug may possibly be affecting scipy eigs/eigsh results, but just to be clear when I said "so eigs or eigsh or both may fail for this example because of that bug" this was just a speculation that they may fail and I have not tried using them with this example.

If DLAHQR -> DLAHQR2 fixes it

I'll probably not be up for hacking fortran code, but I'd agree with patching it in scipy and sending it upstream as you did earlier with the other ARPACK bug found in scipy.

@argriffing
Copy link
Contributor Author

Added the '__main__' code as an actual test, and moved the ARPACK discussion to #3613.

@argriffing
Copy link
Contributor Author

The error is the 50 minute TravisCI timeout. I guess this PR is finished; it modernizes the lobpcg code by replacing vestigial symeig references with eigh calls, and it renovates bit-rotted benchmarks and tests.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling e61d2cc on argriffing:remove-symeig into 7d74cc6 on scipy:master.

@rc
Copy link
Contributor

rc commented May 6, 2014

@argriffing Thanks for cleaning that up!

@argriffing
Copy link
Contributor Author

I may merge this soon if there are no further comments and no objection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants