Implement generic testing from #6343 for matrices

jasongrout commented 15 years ago

The patch calls TestSuite().run() every time load(dumps()) was called in the matrix directory. I also renamed and adapted an existing _test_pickle() into _test_reduce().

CC: @kcrisman @nthiery @hivert @rbeezer

Component: linear algebra

Keywords: TestSuite

Author: Jason Grout

Reviewer: Nicolas M. Thiéry

Merged: sage-4.3.1.rc1

Issue created by migration from https://trac.sagemath.org/ticket/6936

jasongrout commented 15 years ago

comment:2

This, of course, depends on the patches at #6343.

kcrisman commented 15 years ago

comment:4

Ticket #6934 is related, of course.

nthiery commented 15 years ago

Changed keywords from none to TestSuite

nthiery commented 15 years ago

Reviewer: Nicolas M. Thiéry

nthiery commented 15 years ago

comment:5

Hi Jason,

I am glad to hear that TestSuite is useful, even before the category code gets in :-)

Some suggestions:

remove the loads(dumps(a)) == a tests, since anyway they are done by TestSuite
Maybe rename _test_matrix_pickle by _test_reduce?

In fact, it would be nice to have this test whenever __reduce__ is implemented (even though it's tested by test_pickle anyway). An option would be to lift _test_reduce to SageObject, and add a conditional in _test_reduce to do nothing if _reduce_ is not defined. The downside is that, if someone inadvertently remove __reduce__, then this won't be detected. Any better idea?
About _test_minpoly (and in general _test methods for elements): One thing which is not yet clear to me is whether the _test method should be on the elements or the parent. The former sounds more natural; however, with the later, one can use the information from the parent to build a good set of elements on which to run the test.

One option would be to add to each parent a method _test_elements that would run TestSuite on some_elements(). However, there typically might be very expensive test methods that one will want to run on just a couple small elements, and other test methods that one will want to test with many large ones.

But that's just food for thoughts for the long run

Other than that, I will be very happy being a reviewer for this patch, and put a positive review.

I haven't run the tests yet. But from the comments above, I gather that some tests are broken, not because this patch is wrong, but because it uncovers bugs elsewhere. Should those be fixed before setting a positive review on this one?

jasongrout commented 15 years ago

comment:7

Replying to @nthiery:

Hi Jason,

I am glad to hear that TestSuite is useful, even before the category code gets in :-)

Some suggestions:

remove the loads(dumps(a)) == a tests, since anyway they are done by TestSuite

The -review.patch patch does this.

Maybe rename _test_matrix_pickle by _test_reduce?

The -review.patch patch does this.

In fact, it would be nice to have this test whenever __reduce__ is implemented (even though it's tested by test_pickle anyway). An option would be to lift _test_reduce to SageObject, and add a conditional in _test_reduce to do nothing if _reduce_ is not defined. The downside is that, if someone inadvertently remove __reduce__, then this won't be detected. Any better idea?

About _test_minpoly (and in general _test methods for elements): One thing which is not yet clear to me is whether the _test method should be on the elements or the parent. The former sounds more natural; however, with the later, one can use the information from the parent to build a good set of elements on which to run the test.

One option would be to add to each parent a method _test_elements that would run TestSuite on some_elements(). However, there typically might be very expensive test methods that one will want to run on just a couple small elements, and other test methods that one will want to test with many large ones.

But that's just food for thoughts for the long run

The rest of these items sound controversial enough that they should be brought up on sage-devel.

Other than that, I will be very happy being a reviewer for this patch, and put a positive review.

If you're happy with the changes, can you mark this positive review?

I haven't run the tests yet. But from the comments above, I gather that some tests are broken, not because this patch is wrong, but because it uncovers bugs elsewhere. Should those be fixed before setting a positive review on this one?

I'd say merge this, since it uncovers existing bugs, but doesn't introduce any. With this merged, those bugs will get fixed before the next release.

nthiery commented 15 years ago

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
-The patch calls TestSuite().run() every time load(dumps()) is called in the matrix directory.  I also added a _test_minpoly() function, and fixed an existing _test_pickle() function.
+The patch calls TestSuite().run() every time load(dumps()) was called in the matrix directory.  I also added a _test_minpoly() function, and renamed and adapted an existing _test_pickle() into _test_reduce().

 Running doctests in matrix/*.pyx now fails for a number of files.  In other words, this testing infrastructure works and is uncovering bugs!

nthiery commented 15 years ago

comment:8

The -review.patch patch does this.

Thanks!

The rest of these items sound controversial enough that they should be brought up on sage-devel.

Do you mind doing it?

If you're happy with the changes, can you mark this positive review?

Done! However, please someone run the tests. I don't have a sage devel under hand.

I'd say merge this, since it uncovers existing bugs, but doesn't introduce any. With this merged, those bugs will get fixed before the next release.

Sounds good to me.

7c09a680-e216-4024-bb8e-9bfd4aa7f313 commented 15 years ago

comment:9

With trac-6936-matrix-generic-doctesting-minpoly.patch, I got a hunk failure:

[mvngu@sage sage-main]$ hg qimport https://github.com/sagemath/sage/files/ticket6936/trac-6936-matrix-generic-doctesting-minpoly.patch.gz && hg qpush
adding trac-6936-matrix-generic-doctesting-minpoly.patch to series file
applying trac-6936-matrix-generic-doctesting-minpoly.patch
patching file sage/matrix/matrix2.pyx
Hunk #2 FAILED at 1190
1 out of 2 hunks FAILED -- saving rejects to file sage/matrix/matrix2.pyx.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
Errors during apply, please fix and refresh trac-6936-matrix-generic-doctesting-minpoly.patch

The hunk in question is

[mvngu@sage sage-main]$ cat sage/matrix/matrix2.pyx.rej
--- matrix2.pyx
+++ matrix2.pyx
@@ -1190,6 +1191,22 @@
         return mp

+    def _test_minpoly(self, **options):
+        """
+        Checks that :meth:`minpoly` works.
+
+        EXAMPLES::
+
+            sage: a=matrix([[1,2],[3,4]])
+            sage: a._test_minpoly()
+
+        """
+        if self.nrows()==self.ncols():
+            tester = self._tester(**options)
+            # At least we'll check that the minimal polynomial kills the
+            # matrix.
+            tester.assert_(self.minpoly().subs(x=self).is_zero())
+
     def charpoly(self, var='x', algorithm="hessenberg"):
         r"""
         Return the characteristic polynomial of self, as a polynomial over

jasongrout commented 15 years ago

comment:10

Okay, it's rebased; the problem was that "charpoly" changed its default algorithm to "None". Since that doesn't affect anything in this patch, I'll set it back to positive review.

mwhansen commented 15 years ago

Merged: sage-4.2.alpha0

mwhansen commented 15 years ago

comment:12

I get failures in these files:

        sage -t  devel/sage-main/sage/matrix/matrix_generic_dense.pyx # 1 doctests failed
        sage -t  devel/sage-main/sage/matrix/matrix_integer_2x2.pyx # 1 doctests failed
        sage -t  devel/sage-main/sage/matrix/matrix_dense.pyx # 1 doctests failed
        sage -t  devel/sage-main/sage/matrix/matrix1.pyx # 1 doctests failed
        sage -t  devel/sage-main/sage/matrix/matrix_symbolic_dense.pyx # 1 doctests failed

One of them is definitely #5639.

jasongrout commented 15 years ago

comment:13

Aren't these bugs that are exposed by this patch, rather than bugs caused by this patch? If so, then new tickets should be opened, rather than changing this to needs work.

hivert commented 15 years ago

comment:14

Hi there,

When creating #5274 it was my intend to implement this generic testing. So one probably wan't to close #5274 as a duplicate. However, It should be a good idea to include test_trivial_matrices_inverse from matrix_space.py in this generic testing infrastruture, since it currently test the different space by hand.

Cheers,

Florent

jasongrout commented 14 years ago

Attachment: trac-6936-matrix-generic-doctesting.patch.gz

jasongrout commented 14 years ago

Description changed:

--- 
+++ 
@@ -1,5 +1,2 @@
-The patch calls TestSuite().run() every time load(dumps()) was called in the matrix directory.  I also added a _test_minpoly() function, and renamed and adapted an existing _test_pickle() into _test_reduce().
+The patch calls TestSuite().run() every time load(dumps()) was called in the matrix directory.  I also renamed and adapted an existing _test_pickle() into _test_reduce().

-Running doctests in matrix/*.pyx now fails for a number of files.  In other words, this testing infrastructure works and is uncovering bugs!
-
-The bugs should be dealt with on other tickets.

jasongrout commented 14 years ago

Changed merged from sage-4.2.alpha0 to none

jasongrout commented 14 years ago

comment:15

Okay, I took out the minpoly test, so this patch just implements the generic testing infrastructure. The minpoly test will go on another ticket so the doctest failures can be dealt with there.

mwhansen commented 14 years ago

comment:16

This looks good to me. Please put the new ticket number for minpoly here.

jasongrout commented 14 years ago

comment:17

The minpoly ticket is #7989.

89c6e537-b2e3-45e6-882d-d4957b74ffe5 commented 14 years ago

Merged: sage-4.3.1.rc1

sagemath / sage

Implement generic testing from #6343 for matrices #6936