Opened 10 years ago
Closed 6 years ago
#10779 closed enhancement (fixed)
Improve coverage test for structure/element.pyx
Reported by:  ejeanvoi  Owned by:  mvngu 

Priority:  major  Milestone:  sage6.5 
Component:  doctest coverage  Keywords:  beginner 
Cc:  Merged in:  
Authors:  Emmanuel Jeanvoine, Frédéric Chapoton  Reviewers:  Nathann Cohen, Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  dbf722e (Commits, GitHub, GitLab)  Commit:  dbf722e129b8d5239d52f8e202cb11dfeccb7049 
Dependencies:  Stopgaps: 
Description (last modified by )
Improve coverage test for structure/element.pyx
Status as of Sage 6.2.beta4:
SCORE src/sage/structure/element.pyx: 27.5% (42 of 153)
with patch:
SCORE src/sage/structure/element.pyx: 32.7% (50 of 153)
Attachments (2)
Change History (29)
Changed 10 years ago by
comment:1 Changed 8 years ago by
 Priority changed from trivial to major
 Status changed from new to needs_review
comment:2 Changed 8 years ago by
 Description modified (diff)
 Keywords beginner added
 Status changed from needs_review to needs_work
Hello!
 Please refer to the conventions page for how to write in ReST markup. For instance, you need a blank line after
::
for getting verbatim environment. Also, please note that, a verbatim environment is preceded by::
.
 It'd be nice if you also go ahead and add more doctests. Also, please correct the docstrings formatting whenever you can  like codifying
self
,True
and such by using two backticks...
 Major: "ERROR: Please add a
TestSuite(s).run()
doctest."
Hoping to hear from you soon...
Cheers, Kannappan.
comment:3 Changed 8 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:4 Changed 8 years ago by
 Description modified (diff)
comment:5 Changed 8 years ago by
apply only trac_10779doctestsv2.patch
comment:6 Changed 7 years ago by
 Branch set to u/chapoton/10779
 Commit set to c84246e912c9380936c9e122934ed94ab160453f
New commits:
c84246e  #10779: Improve coverage test for structure/element.pyx

comment:7 Changed 7 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:8 Changed 7 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:9 Changed 7 years ago by
 Commit changed from c84246e912c9380936c9e122934ed94ab160453f to db023d458e9e534000f9bc6a99575c5580e07b3e
Branch pushed to git repo; I updated commit sha1. New commits:
db023d4  Merge branch 'u/chapoton/10779' of ssh://trac.sagemath.org:22/sage into 10779

comment:10 Changed 7 years ago by
 Commit changed from db023d458e9e534000f9bc6a99575c5580e07b3e to 2e30025298097a2c280629135f3f76d973c0b4a5
Branch pushed to git repo; I updated commit sha1. New commits:
2e30025  trac #10779 corrected 3 doctests

comment:11 Changed 7 years ago by
 Status changed from needs_work to needs_review
comment:12 Changed 7 years ago by
 Description modified (diff)
comment:13 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:14 Changed 7 years ago by
 Commit changed from 2e30025298097a2c280629135f3f76d973c0b4a5 to 95319371cc13f9512521043bfe53cfe806df244e
comment:15 Changed 6 years ago by
 Commit changed from 95319371cc13f9512521043bfe53cfe806df244e to 54396cd64b3a1e4dc512be3438e85bfbe0bb5452
Branch pushed to git repo; I updated commit sha1. New commits:
54396cd  Merge branch 'u/chapoton/10779' into 6.4

comment:16 Changed 6 years ago by
 Status changed from needs_review to needs_info
Hello !
This code seems good, but I do not understand the three functions lcm,gcd, and now (with your branch) xgcd in element.pyx. Isn't it more reasonable to import them from sage.ring.arith instead ? (a lazy import if that was the problem)
Nathann
comment:17 Changed 6 years ago by
xgcd was already there. This branch only changes the doc.
comment:18 Changed 6 years ago by
Oh sorry, I made this mistake by reading the diff file.
Okay let's go.
Nathann
comment:19 Changed 6 years ago by
 Reviewers set to Nathann Cohen
 Status changed from needs_info to positive_review
comment:20 Changed 6 years ago by
It is especially important to keep this in mind whenever you move a class down from Python to Cython.
Is this really true? I think Cython is just following the Python convention here...
comment:21 followup: ↓ 22 Changed 6 years ago by
The tests for gcd
, lcm
and xgcd
do not actually test the functions from element.pyx
.
comment:22 in reply to: ↑ 21 Changed 6 years ago by
The tests for
gcd
,lcm
andxgcd
do not actually test the functions fromelement.pyx
.
It's true. Do we create a patch to remove them and import the actual ones ? It's probably only to avoid a direct import.
Nathann
comment:23 Changed 6 years ago by
 Branch changed from u/chapoton/10779 to u/jdemeyer/ticket/10779
 Modified changed from 01/16/15 16:28:11 to 01/16/15 16:28:11
comment:24 Changed 6 years ago by
 Commit changed from 54396cd64b3a1e4dc512be3438e85bfbe0bb5452 to dbf722e129b8d5239d52f8e202cb11dfeccb7049
 Reviewers changed from Nathann Cohen to Nathann Cohen, Jeroen Demeyer
 Status changed from positive_review to needs_review
New commits:
dbf722e  Remove gcd, lcm, xgcd from element.pyx

comment:25 Changed 6 years ago by
 Status changed from needs_review to positive_review
Passes all long tests, does the job.
Nathann
comment:26 Changed 6 years ago by
 Milestone changed from sage6.4 to sage6.5
comment:27 Changed 6 years ago by
 Branch changed from u/jdemeyer/ticket/10779 to dbf722e129b8d5239d52f8e202cb11dfeccb7049
 Resolution set to fixed
 Status changed from positive_review to closed
Added some doctests