Improve doctest coverage for sage.rings.qqbar

[loefflerd]

... from its current abysmal 45% (112 of 244) to something a bit more respectable.

Part of the meta-ticket #12024. See also #12665 (a bug discovered in working on this ticket).

CC: @sagetrac-JStarx

Component: doctest coverage

Author: David Loeffler

Reviewer: Jim Stark

Merged: sage-5.0.beta11

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

[loefflerd]

Attachment: trac_12662-qqbar-doctest.patch.gz

Patch against 5.0.beta7

[loefflerd]

Author: David Loeffler

[loefflerd]

comment:1

Here's a patch which gets full doctest coverage.

[orlitzky]

comment:2

Can't we just get rid of this stuff? If anyone misses it, it's in the mercurial history.

-    def gens(self):
-        return(QQbar_I, )
-
-    def gen(self, n=0):
-        if n == 0:
-            return QQbar_I
-        else:
-            raise IndexError, "n must be 0"
-
-    def ngens(self):
-        return 1
+# more nonsense -- DL
+#
+#    def gens(self):
+#        return(QQbar_I, )
+#
+#    def gen(self, n=0):
+#        if n == 0:
+#            return QQbar_I
+#        else:
+#            raise IndexError("n must be 0")
+#
+#    def ngens(self):
+#        return 1

[sagetrac-JStarx]

comment:3

The functions gen, gens, and ngens override methods from ParentWithGens and _is_valid_homomorphism_ overrides a method from Parent. If you don't override them then calling them will return a NotImplementedError which doesn't seem wise as they are used, for example _is_valid_homomorphism_ is called by the init method of RingHomomorphism_im_gens.

Also I do think they make mathematical sense. A field is generated by a single element, the 1 of the field, and for a homomorphism of fields we must send 1 to 1.

[loefflerd]

Work Issues: uncomment generators methods

[loefflerd]

comment:5

All right, I'll put them back in.

[loefflerd]

Apply only this patch. Patch against 5.0.beta10

[loefflerd]

Changed work issues from uncomment generators methods to none

[loefflerd]

comment:6

Attachment: trac_12662-qqbar-doctest-v2.patch.gz

Here's a new patch which adds doctests for these routines instead of commenting them out.

[sagetrac-JStarx]

Work Issues: doctest failure

[sagetrac-JStarx]

comment:7

I applied this to beta10, running OSX 10.6.8, and got the following two failures:

~/sage-dev/devel/sage-main Starx$ ../../sage -t sage/rings/qqbar.py
sage -t  "devel/sage-main/sage/rings/qqbar.py"              
**********************************************************************
File "/Users/Starx/sage-dev/devel/sage-main/sage/rings/qqbar.py", line 535:
    sage: sage.rings.qqbar._late_import()
Exception raised:
    Traceback (most recent call last):
      File "/Users/Starx/sage-dev/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/Users/Starx/sage-dev/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/Users/Starx/sage-dev/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_1[2]>", line 1, in <module>
        sage.rings.qqbar._late_import()###line 535:
    sage: sage.rings.qqbar._late_import()
    AttributeError: 'module' object has no attribute '_late_import'
**********************************************************************
File "/Users/Starx/sage-dev/devel/sage-main/sage/rings/qqbar.py", line 2128:
    sage: qq_generator.pari_field()
Expected:
    Traceback (most recent call last):
    ...
    ValueError: No PARI field attached to trivial generator
Got:
    Traceback (most recent call last):
      File "/Users/Starx/sage-dev/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/Users/Starx/sage-dev/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/Users/Starx/sage-dev/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_68[3]>", line 1, in <module>
        qq_generator.pari_field()###line 2128:
    sage: qq_generator.pari_field()
      File "/Users/Starx/sage-dev/local/lib/python/site-packages/sage/rings/qqbar.py", line 1670, in pari_field
        pari_pol = self._field.pari_polynomial("y")
      File "parent.pyx", line 875, in sage.structure.parent.Parent.__getattr__ (sage/structure/parent.c:6731)
      File "parent.pyx", line 326, in sage.structure.parent.getattr_from_other_class (sage/structure/parent.c:3247)
    AttributeError: 'RationalField_with_category' object has no attribute 'pari_polynomial'
**********************************************************************
2 items had failures:
   1 of   5 in __main__.example_1
   1 of  12 in __main__.example_68
***Test Failed*** 2 failures.

[loefflerd]

comment:8

Dear JStarx,

I strongly suspect you applied the patch and ran "sage -t" without running "sage -b" first, which gives exactly these two errors. (They come from the two non-docstring changes in the patch: I renamed late_import to _late_import so it wouldn't appear in the reference manual, and added one line to AlgebraicGenerator.pari_polynomial so it gave a more informative error message when called on the rational field.)

Can you run "sage -b" and then do the test again?

[loefflerd]

Changed work issues from doctest failure to none

[sagetrac-JStarx]

comment:9

Yup, silly mistake. All the tests pass. It's getting late, but when I get some time this week I'll finish reading through the diff and review it.

[sagetrac-JStarx]

comment:10

Ok, it all looks good. The only minor tweek I would make is to delete the commented stuff on lines 7594 - 7641. Not sure why that's there or why we wouldn't delete it.

Anyway, I'm gonna set it as a positive review. If you agree that that commented stuff can be deleted then feel free to keep the ticket marked as a positive review after deleting it.

[sagetrac-JStarx]

Reviewer: Jim Stark

[jdemeyer]

Merged: sage-5.0.beta11