Opened 11 years ago

Closed 11 years ago

Last modified 5 years ago

#5734 closed defect (fixed)

[with patch, with positive review] Bring doctests of modular/modsym/manin_symbols.py up to 100%

Reported by: cremona Owned by: mabshoff
Priority: major Milestone: sage-3.4.1
Component: doctest coverage Keywords: modular symbols
Cc: davidloeffler, mtaranes Merged in: 3.4.1.rc3
Authors: John Cremona Reviewers: David Loeffler
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by chapoton)

As of 3.4.1.rc1, we have

SCORE /home/jec/sage-3.4.1.rc1/devel/sage-main/sage/modular/modsym/manin_symbols.py: 2% (2 of 68)

Missing documentation:
 * is_ManinSymbol(x):
 * __init__(self, weight, list):
 * __cmp__(self, right):
 * __getitem__(self, n):
 * __len__(self):
 * apply(self, j, X):
 * apply_S(self, j):
 * apply_I(self, j):
 * apply_T(self, j):
 * apply_TT(self, j):
 * manin_symbol_list(self):
 * manin_symbol(self, i):
 * normalize(self, x):
 * weight(self):
 * __init__(self, level, weight, syms):
 * apply_T(self, j):
 * apply_TT(self, j):
 * level(self):
 * normalize(self, x):
 * __init__(self, level, weight):
 * __repr__(self):
 * __init__(self, level, weight):
 * __repr__(self):
 * __init__(self, group, weight):
 * __repr__(self):
 * __init__(self, character, weight):
 * __repr__(self):
 * apply_T(self, j):
 * apply_TT(self, j):
 * character(self):
 * level(self):
 * normalize(self, x):
 * __init__(self, level, weight):
 * __repr__(self):
 * apply_T(self, j):
 * apply_TT(self, j):
 * level(self):
 * normalize(self, x):
 * tuple(self):
 * __get_i(self):
 * __get_u(self):
 * __get_v(self):
 * _repr_(self):
 * _latex_(self):
 * __cmp__(self, other):
 * __mul__(self, matrix):
 * copy(self):
 * parent(self):
 * weight(self):
 * _print_polypart(i, j):

Missing doctests:
 * index(self, x):
 * apply_S(self, j):
 * apply_I(self, j):
 * apply(self, j, m):
 * apply_S(self, j):
 * apply_I(self, j):
 * index(self, x):
 * apply_S(self, j):
 * apply_I(self, j):
 * apply_J(self, j):
 * apply(self, j, m):
 * __init__(self, parent, t):
 * apply(self, a,b,c,d):
 * lift_to_sl2z(self, N):
 * endpoints(self, N=None):
 * modular_symbol_rep(self):

and I think I might have the right background to fix this!

Attachments (2)

trac_5734.patch (60.5 KB) - added by cremona 11 years ago.
trac_5734-2.patch (9.3 KB) - added by davidloeffler 11 years ago.
apply over previous patch

Download all attachments as: .zip

Change History (9)

comment:1 Changed 11 years ago by cremona

  • Cc davidloeffler mtaranes added

David, I thought you might like this one too (when I have done it) -J

Changed 11 years ago by cremona

comment:2 Changed 11 years ago by cremona

  • Summary changed from Bring doctests of modular/modsym/manin_symbols.py up to 100% to [with patch, needs review] Bring doctests of modular/modsym/manin_symbols.py up to 100%

The patch touches only sage/modular/modsym/manin_symbols.py, based on 3.4.1.rc2.

----------------------------------------------------------------------
sage/modular/modsym/manin_symbols.py
SCORE sage/modular/modsym/manin_symbols.py: 100% (59 of 59)
----------------------------------------------------------------------

I also fixed some small bugs in functions which did not seem to be used, removed (by commenting out) a class x__ManinSymbolList_gamma1 which was not used, and implemented a slightly more sensible cmp() function for class ManinSymbol? (which used to return 1 for any two distinct symbols). All doctests in sage/modular pass, and I also checked quite thoroughly through the resulting reference manual page, which also looks good.

Review please!

Changed 11 years ago by davidloeffler

apply over previous patch

comment:3 Changed 11 years ago by davidloeffler

  • Description modified (diff)

This looks great: applies fine to 3.4.1.rc2, all doctests pass, and documentation builds happily.

I've made a few slight adjustments to the formatting (mainly enforcing a consistent convention that INPUT: is always followed by a bulleted list and OUTPUT: never is, which was true for most but not all of the new doctests), and I've added an x == loads(dumps(x)) doctest for each of the classes that are actually intended for direct use. (There was a loads(dumps()) test in the file already, so sage -coverage didn't protest, but it was commented out, and it was for a different class anyway.)

John: if you're happy with the changes I've made, feel free to change this to "positive review".

comment:4 Changed 11 years ago by cremona

  • Summary changed from [with patch, needs review] Bring doctests of modular/modsym/manin_symbols.py up to 100% to [with patch, with positive review] Bring doctests of modular/modsym/manin_symbols.py up to 100%

I am happy. Thanks for going through this carefully. I also think that the conventions to be followed in these new-style docstrings should be written down somewhere!

comment:5 Changed 11 years ago by mabshoff

  • Milestone changed from sage-3.4.2 to sage-3.4.1
  • Resolution set to fixed
  • Status changed from new to closed

Merged both patches in Sage 3.4.1.rc3.

Cheers,

Michael

comment:6 Changed 11 years ago by davidloeffler

  • Authors set to John Cremona
  • Merged in set to 3.4.1.rc3
  • Reviewers set to David Loeffler

comment:7 Changed 5 years ago by chapoton

  • Description modified (diff)
  • Report Upstream set to N/A
Note: See TracTickets for help on using tickets.