Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#12585 closed enhancement (fixed)

Bring matrix/matrix0.pyx to 100% coverage

Reported by: hthomas Owned by: mvngu
Priority: major Milestone: sage-5.0
Component: doctest coverage Keywords:
Cc: hthomas 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:

Description (last modified by kcrisman)

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 hthomas 7 years ago.
trac_12585-review.patch (3.9 KB) - added by davidloeffler 7 years ago.
apply over previous patch

Download all attachments as: .zip

Change History (11)

comment:1 Changed 7 years ago by hthomas

  • Status changed from new to needs_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 7 years ago by hthomas

  • Status changed from needs_review to needs_work

Drat. New patch coming

Changed 7 years ago by hthomas

comment:3 Changed 7 years ago by hthomas

  • Status changed from needs_work to needs_review

This one applies properly to 5.0.beta5.

comment:4 Changed 7 years ago by davidloeffler

  • Reviewers set to 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 7 years ago by davidloeffler

apply over previous patch

comment:5 Changed 7 years ago by davidloeffler

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

(for the patchbot)

comment:6 Changed 7 years ago by kcrisman

  • Description modified (diff)
  • Reviewers changed from David Loeffler to David Loeffler, Karl-Dieter Crisman
  • Status changed from needs_review to positive_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 7 years ago by jdemeyer

  • Merged in set to sage-5.0.beta8
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:8 Changed 7 years ago by hthomas

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 7 years ago by kcrisman

This is now #12664.

Note: See TracTickets for help on using tickets.