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

DOC: Enhance description/usage for np.linalg.eig*h #8098

Merged
merged 2 commits into from
Nov 4, 2016

Conversation

stuartarchibald
Copy link
Contributor

Proposed documentation enhancement to the np.linalg.eig*h
routines.

The current documentation does not reflect the nature of the
underlying LAPACK routines and the way in which they interpet
the input. This documentation and examples are with view of
adding clarity to what is actually performed.

Proposed documentation enhancement to the `np.linalg.eig*h`
routines.

The current documentation does not reflect the nature of the
underlying LAPACK routines and the way in which they interpet
the input. This documentation and examples are with view of
adding clarity to what is actually performed.
@wrwrwr
Copy link
Contributor

wrwrwr commented Oct 16, 2016

EDIT: Obviously, -1 and 4 are eigenvalues for [[0, 2], [2, 3]] and the new example actually demonstrates a similar case.

These functions seem to give hardly explainable results if the matrix is non-symmetric, for instance:

eigvalsh([[0, 1], [2, 3]])
# array([-1.,  4.])

Hence, wouldn't it be better to either:

  • add a check and raise an exception for non-symmetric / non-Hermitian matrices;
  • add an explicit note that in the above cases the result is undefined.

@charris
Copy link
Member

charris commented Nov 4, 2016

@wrwrwr I think the result is well defined, just possibly unexpected. The use of the upper/lower triangular part avoids the numerical difficulty of determining true symmetry and is also compatible with packed storage of Hermitean arrays.

@@ -930,7 +930,11 @@ def eigvalsh(a, UPLO='L'):
computed.
UPLO : {'L', 'U'}, optional
Same as `lower`, with 'L' for lower and 'U' for upper triangular.
Deprecated.
Deprecated.
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this doesn't look deprecated in the code. @pv Is this a stray bit left over from the merge of the gufuncs?

Copy link
Member

Choose a reason for hiding this comment

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

I have no recollection of this, but 74e1477 mentions lower so maybe the idea was to harmonize this with scipy's kwargs and replace the U/L with lower=True/False kwarg.

Copy link
Member

Choose a reason for hiding this comment

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

"Same as lower," looks completely irrelevant. Hmm...

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@pv, OK. I'll remove it until such time, if ever, it gets harmonized with scipy ;) Speaking of which, we might should do the same for the qr decomposition at some point.

The UPLO parameter of eigvalsh was marked as deprecated, apparently in anticipation
of convergence with the scipy version which uses a `lower` boolean parameter. There is
no deprecation in the code, nor a lower parameter, so remove the deprecation.

[ci skip]
@charris charris merged commit 4d300ff into numpy:master Nov 4, 2016
@charris
Copy link
Member

charris commented Nov 4, 2016

Thanks @stuartarchibald .

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.

None yet

4 participants