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: |
Description (last modified by )
Attachments (2)
Change History (13)
Changed 9 years ago by
comment:1 Changed 9 years ago by
- 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
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
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
- Cc jstarx added
comment:5 Changed 9 years ago by
- Status changed from needs_review to needs_work
- Work issues set to uncomment generators methods
All right, I'll put them back in.
comment:6 Changed 9 years ago by
- 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
- 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
- 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
- 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
- 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
- Merged in set to sage-5.0.beta11
- Resolution set to fixed
- Status changed from positive_review to closed
Patch against 5.0.beta7