Opened 11 years ago

Closed 10 years ago

Last modified 6 years ago

#12496 closed enhancement (fixed)

Improve doctest coverage for integer_ring.pyx

Reported by: Samuel Lelièvre Owned by: Samuel Lelièvre
Priority: major Milestone: sage-5.4
Component: documentation Keywords: Cernay2012
Cc: Štěpán Starosta Merged in: sage-5.4.rc0
Authors: Samuel Lelièvre, Hugh Thomas Reviewers: Hugh Thomas, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Attachments (2)

trac_12496-integer_ring-sl2.patch (29.9 KB) - added by Hugh Thomas 10 years ago.
trac_12496-integer_ring-ht2.patch (11.7 KB) - added by Hugh Thomas 10 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 11 years ago by Samuel Lelièvre

Owner: changed from Minh Van Nguyen to Samuel Lelièvre

comment:2 Changed 11 years ago by Štěpán Starosta

Cc: Štěpán Starosta added

comment:3 Changed 11 years ago by Samuel Lelièvre

Status: newneeds_review

This is my first time submitting a patch: feedback welcome!

Added 8 doctests, out of 10 that were missing. The 2 still missing are:

  • __cinit__(self)
  • __richcmp__(left, right, int op)

I have no idea what __cinit__ and __richcmp__ do, when/how they are called, ... and how to write any relevant doctest for them.

The command sage -coverage returns 2 missing docs and 5 possibly wrong (function name doesn't occur in doctests).

comment:4 Changed 11 years ago by Hugh Thomas

Status: needs_reviewneeds_work

Hi!

I am also pretty new to contributing to Sage, and to reviewing. So I have some comments, but they might not all really need to be dealt with. I guess the things that confused me might confuse other people as well, but there's no reason for a non-expert to be looking at some of these things, so maybe it's not really a problem. On the other hand, probably no one will get around to touching this documentation again for a long time, so it makes sense to try to make it as good as possible now.

There are some methods which I couldn't figure out what they were supposed to do (mathematically speaking): degree, absolute_degree, parameter.

There are some methods for which it would be useful (I think) to explain more clearly what the arguments are.

  • gen: what is n?
  • completion what is prec? (well, I figured out it was precision)
  • crt_basis: what does the xgcd parameter do?
  • __getitem__: what is x?
  • _is_valid_homomorphism_: what are the arguments?
  • extension: what are the arguments?

There are a few places where I wanted to change the wording:

  • "to a void" (in cdef void): I think this should be "to avoid"
  • "The default distribution is on average 50% \pm 1" (in random_element): I'm not sure what this means. If it means "is either 1 or -1 50% of the time on average" then it contradicts the discussion above, which says each of 1, 0, -1 occurs 20% of the time. Also, maybe it would be good to include some link to where the reader could find out about the available distributions.
  • "Return the order in the number field defined by poly generated (as a ring) by a root of poly." (in extension) I had trouble making sense of this. I think something like "Given a polynomial as input, return the order generated by a root of the polynomial in the number field defined by the polynomial." would be clearer.

_randomize_mpz could do with more detailed documentation (I think).

_cmp_ could also do with more documentation -- I couldn't figure out what it was doing.

comment:5 Changed 11 years ago by Hugh Thomas

Description: modified (diff)
Reviewers: Hugh Thomas
Status: needs_workneeds_review

I fixed most of the things I was complained about previously. Like the original author, I do not understand __richcmp__ or __cinit__ well enough to write sensible documentation (or tests) for them.

I am still perplexed about _cmp_ is doing.

It is clear that parameter() is returning 1, but I don't understand what it means.

Since neither the original author nor I are very experienced, perhaps the best thing would be if someone else could review both our patches. (Alternatively, Samuel, if you agree with my patches, I am also willing to agree to yours, so we can set the ticket to postive review. But I think it might be better to see if someone else will look at it first.)

comment:6 Changed 11 years ago by Hugh Thomas

Description: modified (diff)

comment:7 Changed 11 years ago by Hugh Thomas

Description: modified (diff)

The patchbot doesn't seem to be attempting to build these patches. Maybe my instruction was unclear? I will try changing it.

comment:8 Changed 11 years ago by Jim Stark

A lot of functions are missing their input/output blocks, which should be added if we're trying to improve the documentation. The functions random_element and completion especially have input specifications but aren't in the right form.

The conventions for documentation strings are here. If you fix the two functions I mentioned (and feel free to fix more than that :) ) I'll give the ticket a positive review.

comment:9 in reply to:  8 Changed 11 years ago by Hugh Thomas

Hi Jim--

Thanks for your review. If the original author doesn't do so, I will try to implement the changes you suggest.

cheers,

Hugh

comment:10 Changed 10 years ago by Jeroen Demeyer

Please fill in your real name as Author.

Changed 10 years ago by Hugh Thomas

comment:11 Changed 10 years ago by Hugh Thomas

Authors: Samuel Lelievre, Hugh Thomas
Description: modified (diff)

comment:12 Changed 10 years ago by Hugh Thomas

I have rebased the patches to apply to 5.2.rc1.

@Jeroen: The original author has not been active on the ticket since submitting the original patch. I added his name as Author per your request and also mine (since I've added a fair bit).

@Jim: I took care of your specific complaints, and made some additional changes along the same lines. I admit that further work could still be done.

Changed 10 years ago by Hugh Thomas

comment:13 Changed 10 years ago by Hugh Thomas

Apply trac_12496-integer_ring-sl2.patch, trac_12496-integer_ring-ht2.patch.

comment:14 Changed 10 years ago by Jeroen Demeyer

Reviewers: Hugh ThomasHugh Thomas, Jeroen Demeyer
Status: needs_reviewpositive_review

There certainly still is room for improvement, but as a first approximation, this patch looks good.

comment:15 Changed 10 years ago by Hugh Thomas

Thanks, Jeroen!

comment:16 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.4.rc0
Resolution: fixed
Status: positive_reviewclosed

comment:17 Changed 6 years ago by Frédéric Chapoton

Authors: Samuel Lelievre, Hugh ThomasSamuel Lelièvre, Hugh Thomas
Note: See TracTickets for help on using tickets.