TST: Fix various incorrect linalg tests #8369

eric-wieser · 2016-12-12T14:31:57Z

The test cases for linalg supposedly include an empty test case. However, this does not actually test the right thing, since this case is supposed to be square, but is not.

This is because atleast_2d(array([], dtype=double)) returns an array of shape (1, 0), not (0, 0)

Not only that, but when a test is marked as working for both square and non-square inputs, only the square inputs are run, because the non-square ones are shadowed by a copy-and-paste-o. As a result, the non-square inputs don't all even make sense, because they were never being run.

After fixing the tests, fixes a bunch of minor bugs exposed by the updated tests, where instead of LinAlgError being thrown, some much-more-internal error was escaping, that hid the cause of the problem.

eric-wieser · 2016-12-12T16:08:21Z

Should LinalgNonsquareTestCase be a subclass of LinalgTestCase?

eric-wieser · 2016-12-12T19:10:50Z

numpy/linalg/tests/test_linalg.py

@@ -338,7 +338,7 @@ def test_sq_cases(self):

 class LinalgNonsquareTestCase(object):

-    def test_sq_cases(self):


This meant that none of the non-square test cases in TestLstsq were actually being run...

Now that they are run, they fail, because

>>> import numpy as np >>> a = np.random.rand(8, 11) >>> b = np.random.rand(11) >>> np.linalg.lstsq(a, b)

doesn't make any sense

Has been fixed.

eric-wieser · 2016-12-13T16:28:43Z

numpy/linalg/linalg.py

+            raise RuntimeError(
+                'LAPACK did not compute workspace sizes: len(work) = {}, len(iwork) = {}'
+                .format(lwork, liwork)
+            )


This is a known and fixed (9 Jun 2010) bug in LAPACK: http://icl.cs.utk.edu/lapack-forum/archives/lapack/msg00899.html/. The scipy.linalg code has a very similar test, that decides to just error out if this bug is present in LAPACK.

This condition is not hit on my machine (Windows, Intel MKL).

However, this fails on Appveyor. What version of LAPACK is being used there, and do we really need to support a version released before python 2.7.0?

It turns out that Accelerate based problems creep up everywhere, I've recently bitten by the same lapack version problem -> Scipy discussion scipy/scipy#6051

@ilayn : It turns out the culprit here is the LAPACK 3.0.0 that is bundled in np.linalg.lapack_lite, which I'm looking at in #8376

Oh, that's indeed prehistoric.

pv · 2016-12-19T13:25:52Z

The description of this PR doesn't actually say what it does. What was incorrect, and how does this address it?

eric-wieser · 2016-12-19T13:30:06Z

@pv: I've updated the description. There was a little scope creep here from the ~~commits after 72a3ca4~~ - I'll cut those back in the next day or two to remedy that. Unfortunately, the only debugging tool I had available to me for the problem I was seeing was the CI servers

eric-wieser · 2016-12-19T14:07:33Z

numpy/linalg/linalg.py

+    #    `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 )


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.

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)

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 ).

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

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!

atleast_2d(array([], dtype=double)) returns an array of shape (1, 0), not (0, 0)

SVD was previously being too generous with accuracy on doubles. This allows pinv, the test for which was previously was too imprecise, to pass.

Allows each individual function to inspect the flags of a certain test, and decide whether an exception will be thrown

These testcases were never used in the first place, due to a typo. This makes their dimensions match the order of the other test cases, even though those also did not run

Throw LinAlgError instead with an appropriate message

…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.

Throw LinAlgError instead with an appropriate message

eric-wieser · 2016-12-19T16:05:43Z

@pv: Squashed, trimmed-in-scope, and passing. Let me know if anything is still unclear

charris · 2017-01-23T01:40:27Z

numpy/linalg/tests/test_linalg.py

@@ -61,30 +61,44 @@ def get_rtol(dtype):

 class LinalgCase(object):

-    def __init__(self, name, a, b, exception_cls=None):
+    def __init__(self, name, a, b, flags=frozenset()):


The class should be documented, at least for the constructor. This is especially so now with the rather generic flags.

Why not a simple list instead of a frozenset? I don't see that an immutable set really matters here.

Agree on documentation, will do that at some point.

sets make the filtering easier than lists, and frozenset prevents individual tests from modifying the list of tests accidentally.

I'm not saying these are particularly good arguments, but they're what I came up with at the time

charris · 2017-01-23T02:17:11Z

numpy/linalg/tests/test_linalg.py

+    def test_nonsq_cases(self):
+        _check_cases(self.do,
+            require=CaseFlags.none,
+            exclude=CaseFlags.generalized | CaseFlags.square | CaseFlags.hermitian | CaseFlags.empty)


See above. I think these would all be clearer as a list of strings and take up less room to boot.

My reason to avoid a list of strings is that making a typo in a string would lead to a test simply not running, rather than an AttributeError

My counter argument would be that it isn't important ;) If you were writing fly-by-wire software that had to be 100% correct at all times, then using an uncommon python feature would be justified. In fact, seeing such usage makes me stop and think and track down why it is important. But here I don't think it matters and I've burned up a couple of precious grey cells to no purpose. The simplest approach that works is what I would chose in this case because it makes the code that much more accessible, and likely more error free as it is easier to check.

charris · 2017-01-23T02:22:07Z

numpy/linalg/tests/test_linalg.py

-    def do(self, a, b):
+    def do(self, a, b, flags):
+        if flags & CaseFlags.empty:
+            assert_raises(LinAlgError, linalg.eigvalsh, a, 'L')


So checking for an error is the default (no flags). That is somewhat unintuitive.

Darn, It turns out empty is a confusing name. Errors only occur when an array has size=0.

charris · 2017-01-23T02:23:52Z

I think the tests mods are overly complicated, which makes them more difficult to follow, which makes them harder to maintain.

Note that the commits are not squashed.

eric-wieser · 2017-02-09T17:49:03Z

@charris I've tried to simplify things now. See the fixup commit above for the diff (I figured it'd be easier to review before applying it). Sorry for the delay on this one

charris · 2017-02-21T17:36:03Z

numpy/linalg/tests/test_linalg.py



 #
 # Gufunc test cases
 #
+def _make_generalized_cases():


Is there really an advantage in having a function with no arguments that operates on a global and is called once? I think the original is cleaner and easier to follow.

Avoiding global namespace pollution - stops anything accidentally using these local variables in a test. Also handy for code folding, sort of

Fair enough.

charris · 2017-02-21T17:37:15Z

numpy/linalg/tests/test_linalg.py

-            GENERALIZED_NONSQUARE_CASES,
-            GENERALIZED_HERMITIAN_CASES):
-
+def _make_strided_cases():


See above. I don't see the use in making this a function.

charris · 2017-02-21T17:42:33Z

numpy/linalg/tests/test_linalg.py

+    """
+    for case in CASES:
+        # filter by require and exclude
+        if case.tags & require != require:


Hmm, this is a bit clumsy for a lookup, although I suspect it would require a redesign to improve it. Probably not worth the effort.

charris · 2017-02-21T17:48:47Z

Couple of style comments. I note this hasn't been squashed, oversight or do you want help?

eric-wieser · 2017-02-21T17:54:43Z

@charris: I partially squashed it, but out of preference of trying to keep logically separate changes, not into one commit. I could probably go a little further

I don't need help performing the squash, but would perha 1241 ps like some advice on what level to squash things to for numpy

charris · 2017-02-21T18:54:56Z

I'd probably have squashed all the TST commits, then added the BUG fixes on top of that. However, as long as none of the commits errors out it is not a big problem. However, not all of the commit messages are up to snuff. For instance

TST: Correct test cases to actually make sense

These testcases were never used in the first place, due to a typo. This makes
their dimensions match the order of the other test cases, even though those
also did not run

Doesn't impart much information because it requires context, it is basically "fix some stuff", whatever "stuff" is. A virtuoso -- Al Viro of Linux fame -- would probably redo the series of commits into a logical rather than temporal series, but we aren't that demanding...

Anyway, I'm tempted to do a squash commit on this.

charris · 2017-02-21T19:39:10Z

In it goes, thanks Eric.

eric-wieser · 2017-02-21T19:39:51Z

Glad I didn't start rebasing those logically then!

eric-wieser · 2017-02-21T19:43:09Z

Thanks, I'll rebase #8649 onto master next time I work on it (and after #8651), so it gets the more thorough testing

eric-wieser · 2017-03-03T02:31:30Z

Oops, looks like we left a !fixup in there

eric-wieser mentioned this pull request Dec 12, 2016

ENH: Implement most linalg operations for 0x0 matrices #8368

Merged

eric-wieser force-pushed the fix-broken-test branch from 4b1147f to e38383a Compare December 12, 2016 15:19

eric-wieser changed the title ~~TST: Correct empty square test case to actually be square, add replac…~~ TST: Correct "empty" square test case to actually be square Dec 12, 2016

eric-wieser changed the title ~~TST: Correct "empty" square test case to actually be square~~ TST: Fix various incorrect linalg tests Dec 12, 2016

charris added 05 - Testing component: numpy.linalg labels Dec 12, 2016

eric-wieser commented Dec 12, 2016

View reviewed changes

eric-wieser force-pushed the fix-broken-test branch 3 times, most recently from bcd1c2a to c1e3c2e Compare December 13, 2016 02:41

eric-wieser mentioned this pull request Dec 13, 2016

Regenerate LAPACK_lite from a newer version of LAPACK #8376

Closed

eric-wieser force-pushed the fix-broken-test branch from 87b784a to d4f37ad Compare December 19, 2016 13:59

eric-wieser commented Dec 19, 2016

View reviewed changes

eric-wieser added 11 commits December 19, 2016 14:32

TST: Correct empty square test case to actually be square

6ceb8a4

atleast_2d(array([], dtype=double)) returns an array of shape (1, 0), not (0, 0)

TST: Add some non-square 0-shaped test-cases

761f8d5

TST: Correct pinv test case such that it doesn't fail correct cases

eb753f8

TST: Prevent non-square testcases being hidden by square ones (fix typo)

21a5142

TST: Enable testing pinv on non-square matrices

6b60a27

TST: Adjust the precision of assert_almost_equal, but based on the type

340779f

SVD was previously being too generous with accuracy on doubles. This allows pinv, the test for which was previously was too imprecise, to pass.

TST: Refactor all the test case lists

c344206

Allows each individual function to inspect the flags of a certain test, and decide whether an exception will be thrown

TST: Correct test cases to actually make sense

b8f75f0

These testcases were never used in the first place, due to a typo. This makes their dimensions match the order of the other test cases, even though those also did not run

BUG: prevent np.linalg.eig ValueError when given a 0x0 array

8356530

Throw LinAlgError instead with an appropriate message

BUG: prevent np.linalg.eigh ValueError when given a 0x0 array

08aa95f

Throw LinAlgError instead with an appropriate message

eric-wieser force-pushed the fix-broken-test branch from f2f1f49 to 08aa95f Compare December 19, 2016 14:34

charris reviewed Jan 23, 2017

View reviewed changes

eric-wieser mentioned this pull request Feb 9, 2017

MAINT: Rebuild lapack lite #8381

Merged

fixup! TST: Refactor all the test case lists

e6d81d9

eric-wieser force-pushed the fix-broken-test branch from 1f6b4ed to e6d81d9 Compare February 9, 2017 18:06

eric-wieser mentioned this pull request Feb 20, 2017

linalg.det throws LinAlgError for 0-by-0 matrices #8212

Closed

charris reviewed Feb 21, 2017

View reviewed changes

charris merged commit 6b9a02a into numpy:master Feb 21, 2017

		@@ -338,7 +338,7 @@ def test_sq_cases(self):

		class LinalgNonsquareTestCase(object):

		def test_sq_cases(self):

Uh oh!

TST: Fix various incorrect linalg tests #8369

TST: Fix various incorrect linalg tests #8369

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!