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: sage-6.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:

Status badges

Description (last modified by chapoton)

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)

trac_10779-doctests.patch​ (3.7 KB) - added by ejeanvoi 10 years ago.
Added some doctests
trac_10779-doctests-v2.patch​ (27.4 KB) - added by chapoton 8 years ago.
replaces previous patch

Download all attachments as: .zip

Change History (29)

Changed 10 years ago by ejeanvoi

Added some doctests

comment:1 Changed 8 years ago by knsam

  • Priority changed from trivial to major
  • Status changed from new to needs_review

comment:2 Changed 8 years ago by knsam

  • 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 jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:4 Changed 8 years ago by chapoton

  • Description modified (diff)

Changed 8 years ago by chapoton

replaces previous patch

comment:5 Changed 8 years ago by chapoton

apply only trac_10779-doctests-v2.patch

comment:6 Changed 7 years ago by chapoton

  • 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 vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:8 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:9 Changed 7 years ago by git

  • Commit changed from c84246e912c9380936c9e122934ed94ab160453f to db023d458e9e534000f9bc6a99575c5580e07b3e

Branch pushed to git repo; I updated commit sha1. New commits:

​db023d4Merge branch 'u/chapoton/10779' of ssh://trac.sagemath.org:22/sage into 10779

comment:10 Changed 7 years ago by git

  • Commit changed from db023d458e9e534000f9bc6a99575c5580e07b3e to 2e30025298097a2c280629135f3f76d973c0b4a5

Branch pushed to git repo; I updated commit sha1. New commits:

​2e30025trac #10779 corrected 3 doctests

comment:11 Changed 7 years ago by chapoton

  • Status changed from needs_work to needs_review

comment:12 Changed 7 years ago by chapoton

  • Authors set to Emmanuel Jeanvoine, Frédéric Chapoton
  • Description modified (diff)

comment:13 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:14 Changed 7 years ago by git

  • Commit changed from 2e30025298097a2c280629135f3f76d973c0b4a5 to 95319371cc13f9512521043bfe53cfe806df244e

Branch pushed to git repo; I updated commit sha1. New commits:

​9d72c81Merge branch 'u/chapoton/10779' of ssh://trac.sagemath.org:22/sage into 10779
​9531937trac #10779 correct failing doctest + more doc changes

comment:15 Changed 6 years ago by git

  • Commit changed from 95319371cc13f9512521043bfe53cfe806df244e to 54396cd64b3a1e4dc512be3438e85bfbe0bb5452

Branch pushed to git repo; I updated commit sha1. New commits:

​54396cdMerge branch 'u/chapoton/10779' into 6.4

comment:16 Changed 6 years ago by ncohen

  • 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 chapoton

xgcd was already there. This branch only changes the doc.

comment:18 Changed 6 years ago by ncohen

Oh sorry, I made this mistake by reading the diff file.

Okay let's go.

Nathann

comment:19 Changed 6 years ago by ncohen

  • Reviewers set to Nathann Cohen
  • Status changed from needs_info to positive_review

comment:20 Changed 6 years ago by jdemeyer 

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 follow-up: Changed 6 years ago by jdemeyer

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 ncohen

The tests for gcd, lcm and xgcd do not actually test the functions from element.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 jdemeyer

  • 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 jdemeyer

  • 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:

​dbf722eRemove gcd, lcm, xgcd from element.pyx

comment:25 Changed 6 years ago by ncohen

  • Status changed from needs_review to positive_review

Passes all long tests, does the job.

Nathann

comment:26 Changed 6 years ago by chapoton

  • Milestone changed from sage-6.4 to sage-6.5

comment:27 Changed 6 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/10779 to dbf722e129b8d5239d52f8e202cb11dfeccb7049
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.