Opened 9 years ago

Closed 9 years ago

#12662 closed defect (fixed)

Improve doctest coverage for sage.rings.qqbar

Reported by: davidloeffler Owned by: mvngu
Priority: major Milestone: sage-5.0
Component: doctest coverage Keywords:
Cc: jstarx Merged in: sage-5.0.beta11
Authors: David Loeffler Reviewers: Jim Stark
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by davidloeffler)

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

Attachments (2)

trac_12662-qqbar-doctest.patch (123.8 KB) - added by davidloeffler 9 years ago.
Patch against 5.0.beta7
trac_12662-qqbar-doctest-v2.patch (142.4 KB) - added by davidloeffler 9 years ago.
Apply only this patch. Patch against 5.0.beta10

Download all attachments as: .zip

Change History (13)

Changed 9 years ago by davidloeffler

Patch against 5.0.beta7

comment:1 Changed 9 years ago by davidloeffler

  • Authors set to David Loeffler
  • Description modified (diff)
  • Status changed from new to needs_review

Here's a patch which gets full doctest coverage.

comment:2 Changed 9 years ago by mjo

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

comment:3 Changed 9 years ago by JStarx

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.

comment:4 Changed 9 years ago by JStarx

  • Cc jstarx added

comment:5 Changed 9 years ago by davidloeffler

  • Status changed from needs_review to needs_work
  • Work issues set to uncomment generators methods

All right, I'll put them back in.

Changed 9 years ago by davidloeffler

Apply only this patch. Patch against 5.0.beta10

comment:6 Changed 9 years ago by davidloeffler

  • Status changed from needs_work to needs_review
  • Work issues uncomment generators methods deleted

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

comment:7 Changed 9 years ago by JStarx

  • Status changed from needs_review to needs_work
  • Work issues set to doctest failure

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.

comment:8 Changed 9 years ago by davidloeffler

  • Status changed from needs_work to needs_info
  • Work issues doctest failure deleted

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?

comment:9 Changed 9 years ago by JStarx

  • Status changed from needs_info to needs_review

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.

comment:10 Changed 9 years ago by JStarx

  • Reviewers set to Jim Stark
  • Status changed from needs_review to positive_review

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.

comment:11 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.0.beta11
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.