Improve doctest coverage for integer_ring.pyx

[slel]

Base ticket #12024: bring doctest coverage to 90%.

---

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

CC: @staroste

Component: documentation

Keywords: Cernay2012

Author: Samuel Lelièvre, Hugh Thomas

Reviewer: Hugh Thomas, Jeroen Demeyer

Merged: sage-5.4.rc0

Issue created by migration from https://trac.sagemath.org/ticket/12496

[slel]

comment:3

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).

[hughrthomas]

comment:4

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.

[hughrthomas]

Reviewer: Hugh Thomas

[hughrthomas]

comment:5

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.)

[hughrthomas]

comment:7

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

[sagetrac-JStarx]

comment:8

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.

[hughrthomas]

comment:9

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

[jdemeyer]

comment:10

Please fill in your real name as Author.

[hughrthomas]

Attachment: trac_12496-integer_ring-sl2.patch.gz

[hughrthomas]

Author: Samuel Lelievre, Hugh Thomas

[hughrthomas]

comment:12

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

@jdemeyer: 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.

[hughrthomas]

Attachment: trac_12496-integer_ring-ht2.patch.gz

[hughrthomas]

comment:13

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

[jdemeyer]

comment:14

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

[jdemeyer]

Changed reviewer from Hugh Thomas to Hugh Thomas, Jeroen Demeyer

[hughrthomas]

comment:15

Thanks, Jeroen!

[jdemeyer]

Merged: sage-5.4.rc0

[fchapoton]

Changed author from Samuel Lelievre, Hugh Thomas to Samuel Lelièvre, Hugh Thomas