More tests could be nice, such as also for the tolerance. Which also makes me wonder, the singular values are not quite the eigenvalues, no? So if you would change to symmetric, the difference, with respect to the tolerance setting, should be more then just numerical accuracy?

Should I make a new method for the symmetric option tests or keep them where they are now?

My last commit fixes the discrepancy by using the absolute value of the eigenvalues, which (as I understand the math) exactly matches the singular values for a symmetric matrix:

• svd(X) = sqrt(eigvals(X.T.dot(X))) in the general case
• X.T.dot(X) = X.dot(X) = matrix_power(X, 2) in the symmetric case
• eigvals(matrix_power(X, k)) = eigvals(X)**k in general

Combining these equalities gives us svd(X) = sqrt(eigvals(X)**2) = abs(eigvals(X)).

Yeah, probably right for symmetry. Do whatever you feel looks nicer, my guess is, it would look nicer in different functions ;) and making tests shorter makes it easier to find the code that broke if it gets triggered.

What is the speed/accuracy advantage?

Isn't the title of this PR incorrect? Looks like matrix_rank is the relevant function. Same with the summary line of the first commit.

@charris: Oops, you're correct on the title mixup. My bad.

Here's some unscientific testing on my Ubuntu machine with master-branch numpy:

In [1]: a = np.random.random((1000, 1000))

In [2]: a = (a + a.T)/2

In [3]: %timeit np.linalg.svd(a, compute_uv=False)
1 loop, best of 3: 218 ms per loop

In [4]: %timeit np.abs(np.linalg.eigvalsh(a))
10 loops, best of 3: 128 ms per loop

I'm not sure how to tell which method is more numerically accurate. They do produce very slightly different results on large badly-scaled matrices, but it's unclear which introduces more floating-point error.

How about the name ishermitean instead?

@charris I changed it to is_hermitian.

☔ The latest upstream changes (presumably #8368) made this pull request unmergeable. Please resolve the merge conflicts.

Would be better as a rebase rather than a merge. Also, this will conflict with #8682 anyway

Given we already have eig + eigh, eigvals + eigvalsh, I think we should either:

• Call the hermitian version matrix_rankh
• Deprecate eigh and eigvalsh and add eig(..., hermitian=True), eigvals(..., hermitian=True)

Yeah, I was trying out Github's web-based conflict resolution tool. It's nice, but a rebase would indeed be cleaner. If the Github folks are watching, maybe that would be a nice feature to add? ;-) I'll rebase and squash when/if this PR is accepted for merging, especially because I need to fix my commit message.

I'm -1 for matrix_rankh, and in general I found the -h suffix versions pretty inscrutable back when I was first learning the API. Actually deprecating eigh and friends seems like it would affect a lot of code in the wild, though...

One benefit of adding a hermitian kwarg would be to eventually enable eig(..., hermitian='autodetect') or similar, which could be convenient for some users and also matches the Matlab behavior.

☔ The latest upstream changes (presumably #8682) made this pull request unmergeable. Please resolve the merge conflicts.

Needs rebase

Rebased and squashed, and updated the title.

Do we want to consider allowing hermitian='U', which forwards to eigvalsh(UPLO='U')?

I'm not sure how useful allowing 'U' and 'L' would be, so I'll defer to the consensus.

I'm not sure how useful allowing 'U' and 'L' would be,

I agree, but we already expose it on eigvals, so there's an argument to keep it. I only mention it as this is probably going to trigger a PR to move all the functions to hermitian=... at some point, and it would be nice to have a roadmap now.

No need to change this PR further

I've opened gh-9436 to get that roadmap out explicitly.

Needs a release note.

Might be nice to add a benchmark comparing hermitian with not, but not necessary.

Added a release note and rebased. I didn't add an asv benchmark because those are more useful for tracking efficiency improvements for the same codepaths. I did run a final sanity check using runtests.py -i, though:

Python 3.5.3 (default, Jan 19 2017, 14:11:04) 
Type 'copyright', 'credits' or 'license' for more information
IPython 6.1.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: x = np.random.random((500, 500))

In [2]: x = (x + x.T) / 2

In [3]: %timeit np.linalg.matrix_rank(x)
157 ms ± 194 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

In [4]: %timeit np.linalg.matrix_rank(x, hermitian=True)
46.7 ms ± 264 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

Rebased to fix merge conflicts.

More conflicts, I'm afraid. I'll try and merge this before it conflicts again

Conflicts resolved.

I don't agree with that last commit - that doesn't seem to match the style of other files.

Can you push again without that commit? Also, I don't think I can restart appveyor with a ci-skip commit last.

Ok, I reverted that change and squashed the commits. Hopefully appveyor will work this time!

Oh I see - the last commit brought it in line with what it was before. Whatever, let's just put this in, since the tests now run. Thanks @perimosocordiae!

Thanks for sticking with this, @eric-wieser !