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

Add Eigenvalue Range Functionality for Symmetric Eigenvalue Problems #6510

Closed
wants to merge 7 commits into from

Conversation

apapanico
Copy link

@apapanico apapanico commented Aug 22, 2016

Updated, renamed, and cleaned up. @apapanico 2016-08-25

Add Eigenvalue Range Functionality for Symmetric Eigenvalue Problems

This PR does the following:

  • As discussed in Computing Eigenvalues in an Interval with LAPACK #6502, changes the interface the -evr LAPACK methods to allow for computing eigenvalues only in a half-open interval specified by the parameters vl and vu.
  • As discussed in Computing Eigenvalues in an Interval with LAPACK #6502, backwards compatibility is ensured by further wrapping the -evr wrappers to reorder the arguments such that vl and vu come at the end of the optional argument list. In scipy/linalg/lapack.py, the Fortran routines are wrapped after import and provided a new doc string to match the appropriate signature. Moreover, a bug is addressed where il and iu are NOT properly ignored in -evr methods when not setting range='I' despite the claims of the LAPACK documentation. For example, if il and iu are set to 1 and range='A', then all eigenvalues are returned but only 1 eigenvector. This is fixed by setting il=1 and iu = n (the size of the matrix) when range='A' or range='V' so that there are no missing eigenvectors. This approach is an improvement on Updating LAPACK -syevr methods #6503 which could not ensure a truly preserved ordering of input arguments.
  • A new keyword argument, eigrng, is added to scipy.linalg.eigh that allows specifying this mode of use. eigrng must be a tuple of length 2 and the first value must be less than the second value. It cannot be specified in conjunction with eigvals. Finally, the return values are all eigenvalues, and eigenvectors if desired, within the range.
  • Tests are written that ensure the wrapper does not break backward compatibility, eigh functionality is as it should be, and that this new mode is incompatible with the mode specified by the parameter eigvals which computes eigenvalues corresponding to an index range.

- Utilizing prefixes
- Changing intent on vl and vu from hide to in
After exposing the parameters vl and vu in the signature for the fortran subroutines, the routines are wrapped to preserve the original ordering of arguments and append vl and vu at the end of the list.
Tests make sure the previously calling format is maintained and vl and vu are pushed to the end of the arg list.
@apapanico
Copy link
Author

apapanico commented Aug 24, 2016

I really don't get why these changes aren't passing. I just did the following:

  • On Docker with Ubuntu 12.04, I went through various python configurations. The library builds find. It's in the testing phase that it fails. However, the test I introduce in 73bba9f does not fail. I put the trace at the bottom of this post.
  • On the Docker setup with Python 3.5.2, I tried to just build the master branch. It failed:
$ python runtests.py -j 4 -gv
...
test_errprint (test_basic.TestErf) ... Segmentation fault
  • I rebuilt master again without parallelism and it was fine.

So I really don't get what's going on. Hopefully it can be worked out. This would be a useful feature to have.

REMOVED

@apapanico
Copy link
Author

A google search for "*** glibc detected *** python: free(): invalid next size (normal)" turns of a lot of results from ~4-6 years ago. Sounds like it has to do with some old libraries or Linux builds. Not sure why it's failing here and not elsewhere.

@pv
Copy link
Member

pv commented Aug 24, 2016

It's typically a sign of memory corruption occurring. As said earlier, these bugs don't always manifest if you're lucky enough nothing important is in the place where stuff gets overwritten. The backtrace it gives is useless, because the crashing free() is a symptom, and the cause is elsewhere.

integer intent(hide),dimension(2*m) :: isuppz
integer intent(in),depend(n) :: lwork=18*n
complex intent(hide),dimension(lwork) :: work
<ftype2c> intent(hide),dimension(lwork) :: work
integer intent(hide),depend(n) :: lrwork=24*n
real intent(hide),dimension(lrwork) :: rwork
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong data type

@pv
Copy link
Member

pv commented Aug 24, 2016

iow, it's a sign of something wrong, and in this case it seems the cause could be found by carefully reading through the change made, as I wrote in the other pull request.

@apapanico
Copy link
Author

Shoot. I'm sorry about this. I was poring over this code so much already I was blind to that. Thanks for the help.

A new parameter `eigrng` is added to `eigh` which allows the specification of computing eigenvalues only within the half-open interval set by `eigrng`.  Checks are done to ensure `eigrng` and `eigvals` are both not set and `eigrng` is a valid range.
Adding tests for the new parameter `eigrng` to accompany previous tests of `eigh`.  Also checking properly raised errors.
@apapanico apapanico changed the title Updating LAPACK -syevr methods via monkeypatch (Alternative/Improvement to #6503) Add Eigenvalue Range Functionality for Symmetric Eigenvalue Problems Aug 25, 2016
Removing extra blank lines
@ev-br ev-br added scipy.linalg enhancement A new feature or improvement labels Sep 14, 2016
Copy link
Member

@pv pv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some changes appear necessary, see individual comments.

(vl, vu) = eigrng
w_full, v_full, info = evr(a1, uplo=uplo, jobz=_job, range="V", vl=vl,
vu=vu, overwrite_a=overwrite_a)
ix = nonzero(w_full)[0].tolist()
Copy link
Member

@pv pv Sep 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if there are zero eigenvalues?

Is the number of eigenvalues found returned by the LAPACK routine in the M parameter?
If not, how is it possible to know how many were found?
What is the purpose of ISUPPZ parameter in the dsyevr?

The nonzero and tolist are probably not needed.

@@ -7,6 +7,7 @@
# additions by Bart Vandereycken, June 2006
# additions by Andrew D Straw, May 2007
# additions by Tiziano Zito, November 2008
# additions by Alex Papanicolaou, August 2016
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't necessarily need to add a comment here --- who modified what is kept track of by the version contorl.
In particular, the file has been modified by many people since 2008.

@@ -240,7 +241,7 @@ def eig(a, b=None, left=False, right=True, overwrite_a=False,

def eigh(a, b=None, lower=True, eigvals_only=False, overwrite_a=False,
overwrite_b=False, turbo=True, eigvals=None, type=1,
check_finite=True):
check_finite=True, eigrng=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to write eig_range or just range instead of abbreviations.

@@ -404,6 +404,73 @@
_flapack.sgegv = sgegv
_flapack.zgegv = zgegv

# Patching EVR Methods to handle value ranges
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should say that this is used to retain backward compatibility.

wrapper.__module__ = 'scipy.linalg._flapack'
wrapper.__doc__ = _evr_doc.format(
name=name, w_type=type.lower(), type=type)
wrapper.__name__ = name
Copy link
Member

@pv pv Sep 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also copy the _cpointer attribute for full backward compat.

for typ in v['dtype']:
for overwrite in v['overwrite']:
for eigenrange in v['eigrng']:
for lower in v['lower']:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that itertools.product can be used to avoid deeply nested loops:

dims = (DIM,)
dtypes = ('f', 'd', 'F', 'D')
...
parts = [dims, dtypes, ...]
for dim, typ, overwrite, eigenrange, lower in itertools.product(*parts):
    yield (...)

ssyevr = evr_decorator(ssyevr, 'ssyevr', 'f')
dsyevr = evr_decorator(dsyevr, 'dsyevr', 'd')
cheevr = evr_decorator(cheevr, 'cheevr', 'F')
zheevr = evr_decorator(zheevr, 'zheevr', 'D')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need also replace the function in _flapack:

_flapack.ssyevr = ssyevr

@@ -27,6 +27,10 @@
from scipy.linalg.lapack import get_lapack_funcs
from scipy.linalg.blas import get_blas_funcs

# Special import for monkey patched functions
from scipy.linalg.lapack import dsyevr, ssyevr, cheevr, zheevr
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change in lapack.py needs to be made so that this is not necessary.
In particular, get_lapack_funcs should return the same functions.

@pv pv added the needs-work Items that are pending response from the author label Sep 17, 2016
@ilayn
Copy link
Member

ilayn commented Dec 23, 2017

Hi @apapanico while investigating #8205 it turned out that lwork, liwork parameters were the culprit. I've done something similar and only then I've seen this PR and the related discussion. From the look of it, it covered quite some distance. But out parameters such as m are still hidden. I think everything should be exposed and then it can be cherrypicked for eigh or other stuff. Currently checks, protoargument and callstatements are missing.

Are you still planning to finish this by any chance? Otherwise, I can take over if you wish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement needs-work Items that are pending response from the author scipy.linalg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants