8000 TST: Fix various incorrect linalg tests by eric-wieser · Pull Request #8369 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

TST: Fix various incorrect linalg tests #8369

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Feb 21, 2017
Merged

TST: Fix various incorrect linalg tests #8369

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
6ceb8a4
TST: Correct empty square test case to actually be square
eric-wieser Dec 12, 2016
761f8d5
TST: Add some non-square 0-shaped test-cases
eric-wieser Dec 12, 2016
eb753f8
TST: Correct pinv test case such that it doesn't fail correct cases
eric-wieser Dec 12, 2016
21a5142
TST: Prevent non-square testcases being hidden by square ones (fix typo)
eric-wieser Dec 12, 2016
6b60a27
TST: Enable testing pinv on non-square matrices
eric-wieser Dec 12, 2016
340779f
TST: Adjust the precision of assert_almost_equal, but based on the type
eric-wieser Dec 12, 2016
c344206
TST: Refactor all the test case lists
eric-wieser Dec 12, 2016
b8f75f0
TST: Correct test cases to actually make sense
eric-wieser Dec 12, 2016
8356530
BUG: prevent np.linalg.eig ValueError when given a 0x0 array
eric-wieser Dec 12, 2016
b28ea1c
BUG: Raise LinAlgError from lstsq rather than a math domain error fro…
eric-wieser Dec 19, 2016
08aa95f
BUG: prevent np.linalg.eigh ValueError when given a 0x0 array
eric-wieser Dec 19, 2016
e6d81d9
fixup! TST: Refactor all the test case lists
eric-wieser Feb 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
BUG: Raise LinAlgError from lstsq rather than a math domain error fro… 
…m math.log

We could make the log conditional on its argument being non-zero, but that entire expression is wrong anyway.
We could omit that calculation entirely and have LAPACK calculate it for us, but the routine in LAPACK is wrong anyway
We could upgrade the version of lapack shipped in lapack_lite, but the tool to do that is wrong anyway.

Let's leave that can of worms for another time, and just improve the error message for now.
  • Loading branch information
@eric-wieser
eric-wieser committed Dec 19, 2016
commit b28ea1c57c9dba8feec7f616986f2e3e28dd6192
9 changes: 9 additions & 0 deletions numpy/linalg/linalg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1919,6 +1919,7 @@ def lstsq(a, b, rcond=-1):
if is_1d:
b = b[:, newaxis]
_assertRank2(a, b)
_assertNoEmpty2d(a, b) # TODO: relax this constraint
m = a.shape[0]
n = a.shape[1]
n_rhs = b.shape[1]
Expand All @@ -1933,6 +1934,14 @@ def lstsq(a, b, rcond=-1):
a, bstar = _fastCopyAndTranspose(t, a, bstar)
a, bstar = _to_native_byte_order(a, bstar)
s = zeros((min(m, n),), real_t)
# This line:
# * is incorrect, according to the LAPACK documentation
# * raises a ValueError if min(m,n) == 0
# * should not be calculated here anyway, as LAPACK should calculate
# `liwork` for us. But that only works if our version of lapack does
# not have this bug:
# http://icl.cs.utk.edu/lapack-forum/archives/lapack/msg00899.html
# Lapack_lite does have that bug...
nlvl = max( 0, int( math.log( float(min(m, n))/2. ) ) + 1 )
Copy link
Member Author

Choose a reason for hiding this comment

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

According to LAPACK, this should be:

SMLSIZ is returned by ILAENV and is equal to the maximum
size of the subproblems at the bottom of the computation
tree (usually about 25), and
NLVL = MAX( 0, INT( LOG_2( MIN( M,N )/(SMLSIZ+1) ) ) + 1 )

For whatever reason, we seem to have decided that log(2) = SMLSIZ = 1, which seems best described as "false". Either way, this is a can of worms, and not one that I think this PR should be opening.

Copy link
Member

Choose a reason for hiding this comment

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

Should we open an issue about this? Is there some example for when this goes bad? (I really hate bugs that might silently create incorrect results, and if this is the case, we should give it some priority)

Copy link
Member Author
@eric-wieser eric-wieser Jan 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I have a patch in the works that uses lapack's internal mechanism to calculate this correctly. Unfortunately, there's a bug in the modified version of lapack bundled with numpy that makes this work. We'd need to regenerate the lapack c code from a newer-but-not-so-new-to-break-f2c release of lapack (#8376). The next step of fixing this is to actually get the generator running again ( #8381 ).

Copy link
Member

Choose a reason for hiding this comment

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

Great, I have no idea of these things, was just worried this might be forgotten :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Guess it would do no harm to open an issue. My working tree was branching way too much for a bunch of problems discovered while trying to fix the problems, and I think the best call is to sit tight and wait for PR merge/rejection, rather than increasing the amount of rebases I need to do each time!

iwork = zeros((3*min(m, n)*nlvl+11*min(m, n),), fortran_int)
if isComplexType(t):
Expand Down
0