#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: |
Description (last modified by )
Base ticket #12024: bring doctest coverage to 90%.
Apply trac_12496-integer_ring-sl2.patch, trac_12496-integer_ring-ht2.patch.
Attachments (2)
Change History (19)
comment:1 Changed 11 years ago by
Owner: | changed from Minh Van Nguyen to Samuel Lelièvre |
---|
comment:2 Changed 11 years ago by
Cc: | Štěpán Starosta added |
---|
comment:3 Changed 11 years ago by
Status: | new → needs_review |
---|
comment:4 Changed 11 years ago by
Status: | needs_review → needs_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
Description: | modified (diff) |
---|---|
Reviewers: | → Hugh Thomas |
Status: | needs_work → needs_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
Description: | modified (diff) |
---|
comment:7 Changed 11 years ago by
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 follow-up: 9 Changed 11 years ago by
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 Changed 11 years ago by
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
Changed 10 years ago by
Attachment: | trac_12496-integer_ring-sl2.patch added |
---|
comment:11 Changed 10 years ago by
Authors: | → Samuel Lelievre, Hugh Thomas |
---|---|
Description: | modified (diff) |
comment:12 Changed 10 years ago by
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
Attachment: | trac_12496-integer_ring-ht2.patch added |
---|
comment:13 Changed 10 years ago by
Apply trac_12496-integer_ring-sl2.patch, trac_12496-integer_ring-ht2.patch.
comment:14 Changed 10 years ago by
Reviewers: | Hugh Thomas → Hugh Thomas, Jeroen Demeyer |
---|---|
Status: | needs_review → positive_review |
There certainly still is room for improvement, but as a first approximation, this patch looks good.
comment:16 Changed 10 years ago by
Merged in: | → sage-5.4.rc0 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
comment:17 Changed 6 years ago by
Authors: | Samuel Lelievre, Hugh Thomas → Samuel Lelièvre, Hugh Thomas |
---|
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).