Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#12585 closed enhancement (fixed)

Bring matrix/matrix0.pyx to 100% coverage

Reported by: Hugh Thomas Owned by: Minh Van Nguyen
Priority: major Milestone: sage-5.0
Component: doctest coverage Keywords:
Cc: Hugh Thomas Merged in: sage-5.0.beta8
Authors: Hugh Thomas Reviewers: David Loeffler, Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Karl-Dieter Crisman)

Improve the doctests for matrix/matrix0.pyx.


Apply trac_12585_matrix0_doc-ht.patch and trac_12585-review.patch.

Attachments (2)

trac_12585_matrix0_doc-ht.patch (6.3 KB) - added by Hugh Thomas 11 years ago.
trac_12585-review.patch (3.9 KB) - added by David Loeffler 11 years ago.
apply over previous patch

Download all attachments as: .zip

Change History (11)

comment:1 Changed 11 years ago by Hugh Thomas

Status: newneeds_review

I think this covers it. sage -coverage still complains about one thing, but I think it's confused (the thing it's found is something defined by cdef but which is not a method).

comment:2 Changed 11 years ago by Hugh Thomas

Status: needs_reviewneeds_work

Drat. New patch coming

Changed 11 years ago by Hugh Thomas

comment:3 Changed 11 years ago by Hugh Thomas

Status: needs_workneeds_review

This one applies properly to 5.0.beta5.

comment:4 Changed 11 years ago by David Loeffler

Reviewers: David Loeffler

The reason sage -coverage was complaining is because of the comment placed *before* the docstring. I.e. it's not clever enough to parse

def some_function(args):
    # comment
    """ docstring """
    function_body

I'm about to upload a tiny reviewer patch which moves the comment in _lmul_ to after the docstring, and thus gets coverage up to 100%. I've also tagged a few doctests as # indirect doctest to stop it complaining about the function name not appearing. Hugh: I'm happy with your patch, so if you're happy with my patch please set this to "positive review".

Changed 11 years ago by David Loeffler

Attachment: trac_12585-review.patch added

apply over previous patch

comment:5 Changed 11 years ago by David Loeffler

Apply trac_12585_matrix0_doc-ht.patch, trac_12585-review.patch

(for the patchbot)

comment:6 Changed 11 years ago by Karl-Dieter Crisman

Description: modified (diff)
Reviewers: David LoefflerDavid Loeffler, Karl-Dieter Crisman
Status: needs_reviewpositive_review

Looks pretty good. Considering that the changes you made after colons were all in underscored functions, so they didn't appear in the reference manual anyway, Hugh did good work too :)

Question:

def unpickle(cls, parent, mutability, cache, data, version):
    r"""
    Unpickle a matrix. This is only used internally by Sage. Users
    should never call this function directly.
    
    EXAMPLES: We illustrating saving and loading several different
    types of matrices.
    
    OVER `\ZZ`::
    
        sage: A = matrix(ZZ,2,range(4))
        sage: loads(dumps(A)) # indirect doctest
        [0 1]
        [2 3]
    
    Sparse OVER `\QQ`:
    
    Dense over `\QQ[x,y]`:
    
    Dense over finite field.
    """

? Since this was there before, I guess it's not necessarily going to make this 'needs work', but I am a little mystified by what the story behind this is. Should we open a ticket for this?

comment:7 Changed 11 years ago by Jeroen Demeyer

Merged in: sage-5.0.beta8
Resolution: fixed
Status: positive_reviewclosed

comment:8 Changed 11 years ago by Hugh Thomas

Thanks for reviewing this, David and Karl-Dieter! Sorry not to have gotten back to you faster --- I was away from email. I am (of course) happy with your changes, David.

I'm not familiar enough with the Sage development process to assess whether it would be useful to open the ticket that you suggest, Karl-Dieter, but I guess it can't do any harm, so please go ahead if you think it would be productive. I don't understand pickling, so I am not likely to do any work on it myself.

comment:9 Changed 11 years ago by Karl-Dieter Crisman

This is now #12664.

Note: See TracTickets for help on using tickets.