Opened 13 years ago

Closed 13 years ago

# Bring doctests for mwrank.pyx up to 100% (from 3%)

Reported by: Owned by: cremona cremona major sage-4.4.4 documentation mwrank rlm sage-4.4.4.alpha0 John Cremona, Minh Van Nguyen, Leif Leonhardy Minh Van Nguyen, Leif Leonhardy, David Loeffler N/A

### GitHub link to the corresponding issue

Improve documentation for this:

sage/libs/mwrank/mwrank.pyx:  3% (1 of 30)


Apply in this order:

### comment:1 Changed 13 years ago by cremona

I am working on this (started 2010-04-28) -- John

### comment:2 Changed 13 years ago by cremona

Authors: → John Cremona

The patch provides 100% doctests to sage/libs/mwrank (both files mwrank.pyx and interface.py), as well as adding them to the reference manual.

This is 95% in the docstrings, but I did find a few fairly minor bugs (mainly in the documentation not saying what various parameters did precisely).

Patch up very soon -- just doing final testing.

Applies to 4.4

### comment:3 follow-up:  4 Changed 13 years ago by cremona

Status: new → needs_review

I am flagging this as "needs review", while still testing. Not expecting any surprises since the patch is almost entirely in docstrings. (I did also test the building of the docs).

### comment:4 in reply to:  3 Changed 13 years ago by cremona

I am flagging this as "needs review", while still testing. Not expecting any surprises since the patch is almost entirely in docstrings. (I did also test the building of the docs).

Just in case anyone reading the above thinks that this is not ready for testing & reviewing -- it is!

### Changed 13 years ago by mvngu

based on Sage 4.4.2.alpha0

### comment:5 Changed 13 years ago by mvngu

With the patch trac_8799-mwrank-doctest.patch, I got the following failures on sage.math:

[mvngu@sage sage-4.4.2.alpha0-8799-mwrank-doctest]\$ ./sage -t -long devel/sage-main/sage/libs/mwrank/mwrank.pyx
sage -t -long "devel/sage-main/sage/libs/mwrank/mwrank.pyx"
**********************************************************************
File "/dev/shm/mvngu/sandbox/sage-4.4.2.alpha0-8799-mwrank-doctest/devel/sage-main/sage/libs/mwrank/mwrank.pyx", line 428:
sage: E.discriminant()
Expected:
-1269581104000000L
Got:
-1269581104000000
**********************************************************************
File "/dev/shm/mvngu/sandbox/sage-4.4.2.alpha0-8799-mwrank-doctest/devel/sage-main/sage/libs/mwrank/mwrank.pyx", line 455:
sage: E.conductor()
Expected:
126958110400L
Got:
126958110400
P1 = [0:1:0]	 is torsion point, order 1
P1 = [-3:0:1]	  is generator number 1
saturating up to 20...Checking 2-saturation

<output truncated>

Searching for points (bound = 8)...done:
found points of rank 3
and regulator 0.41714355875838396981711954461809339674981010609846
Processing points found during 2-descent...done:
now regulator = 0.41714355875838396981711954461809339674981010609846
No saturation being done


The reviewer patch fix this issue and a bunch of typos. What we need now is someone to review the technical aspect of John's patch. Unfortunately, I'm not at all familiar with elliptic curves.

### comment:6 Changed 13 years ago by cremona

Many thanks for the reviewer patch, and many many apologies for all those typose & other things which I missed converting from old-style docstring format to ReST.

For info: the removal of the trailing L on those output constants is correct: I converted various integers in the output to Sage integers (they were python ints).

I CC'd rlm, hoping that he'll be able to look at this too.

### comment:7 follow-ups:  8  9 Changed 13 years ago by leif

Hmm, what's the convention for typesetting numbers, isolated or e.g. in 2-descent?

Math mode looks very ugly (and isn't consistently used).

Also, Python should be uppercase, and Sha should be SHA I guess.

There are also some (non-)references e.g. to 2-descent where I guess :meth:two_descent should be used.

Btw, both patches work for 4.4.1.rc0 (which is almost 4.4.1), too, and doctests pass. I only get an error when building PDF documentation, due to some reference with an underscore(?), haven't yet closely looked at that.

-Leif

### comment:8 in reply to:  7 Changed 13 years ago by leif

I only get an error when building PDF documentation, due to some reference with an underscore(?), haven't yet closely looked at that.

Sorry, perhaps completely unrelated to this patch/ticket.

### comment:9 in reply to:  7 ; follow-up:  10 Changed 13 years ago by leif

Also, ..[...] Sha should be SHA I guess.

Ouch, it's actually the cyrillic Sha. :)

Sorry again.

### comment:10 in reply to:  9 ; follow-up:  11 Changed 13 years ago by cremona

Also, ..[...] Sha should be SHA I guess.

Ouch, it's actually the cyrillic Sha. :)

That's right. I don't know if we can access such fonts in the docs. I think the other inconsistencies have been dealt with by Minh.

John

Sorry again.

### comment:11 in reply to:  10 Changed 13 years ago by leif

I think the other inconsistencies have been dealt with by Minh.

No, not really. Minh introduced some math-typeset numbers - isolated and within words (but did not change all occurrences), and did not "change" all method references.

There's also some reference to Python's (deprecated) long.

The typesetting of Python (variable) names (and e.g. true instead of True) is not fully consistent, too.

(The return value descriptions use both passive and imperative form, too. ;-) )

But I don't want to grumble to much...

### comment:12 Changed 13 years ago by cremona

Feel free to add a second reviewer's patch!

### Changed 13 years ago by leif

A (rough) second reviewer patch based on Minh's.

### comment:13 Changed 13 years ago by leif

Better first look at the diff (before applying the patch); I've changed various things (including a typo in the actual code), but haven't reverted anything Minh changed (at least I think so), i.e. math-typeset numbers etc. still look ugly. ;-)

I've (yet) changed little in mwrank.pyx, because I can't find documentation generated from anything but the two pure Python functions... ;-)

-Leif

P.S.: Minh, the prev/next topic for me doesn't work as expected... 8|

### comment:14 Changed 13 years ago by leif

P.P.S.: John, I've also added some TODOs. Btw, do we initialise or initialize? ;-)

### comment:15 Changed 13 years ago by mvngu

Description: modified (diff) → Minh Van Nguyen, Leif Leonhardy

I have made some more changes to the docstring of interface.py and mwrank.pyx. My second reviewer patch trac_8799-reviewer-part3.patch includes changes in addition to the original reviewer patch and leif's patch. With many patches to apply, it can be confusing to see what changes the reviewers propose. So I have folded the reviewers' patches into one mega patch called trac_8799-reviewer-total.patch.

Also notice that I move documentation for constructor methods, i.e. __init__, into the docstring of a class. The main reason is that, currently the docstring of __init__ don't show up on the reference manual once you built it.

To rebuild the whole reference manual, you could do the following from SAGE_ROOT, assuming you applied the patches to the branch main:

./sage -b main
./sage -docbuild reference html


If you want to be thorough and want to check that the new documentation build OK, do:

rm -rf devel/sage-main/doc/output/
./sage -b main
./sage -docbuild reference html


### Changed 13 years ago by leif

4th reviewer patch on top of Minh' second (-part3). Fixes 1 doctest failure, too.

### comment:16 follow-up:  17 Changed 13 years ago by leif

Sorry, yet another patch. (It also fixes a doctest failure introduced by me. 8/ )

Minh, you vote in favour of math-typeset numbers? (I don't like e.g. L-functions either...)

Shouldn't we replace

 - foo (bool) -- ...
- bar (int, default 12) -- ...


by

 - foo (:class:bool) -- ...
- bar (:class:int, default 12) -- ...


too?

-Leif

P.S.: Note that despite the ticket title, my patches also change the code.

### Changed 13 years ago by mvngu

cumulative of all reviewers' patches

### comment:17 in reply to:  16 ; follow-up:  18 Changed 13 years ago by mvngu

Description: modified (diff)

Minh, you vote in favour of math-typeset numbers? (I don't like e.g. L-functions either...)

I'm not particularly picky about this issue. What you proposed in your patches are OK by me. I have folded all our reviewer patches into one cumulative patch. Even your second reviewer patch has been folded into trac_8799-reviewer-total.patch.

Shouldn't we replace

- foo (bool) -- ...
- bar (int, default 12) -- ...


by

- foo (:class:bool) -- ...
- bar (:class:int, default 12) -- ...


too?

That is OK by me. But I would prefer something like "boolean", "integer", "real number", etc. Something as "meaningful" as possible, without recourse to type information. We now need someone to review the technical (mathematical) aspect of John's patch, and a sign off on my review patch. I'm OK with your patches. That is, someone other than myself need to look over the cumulative reviewer patch.

### comment:18 in reply to:  17 Changed 13 years ago by leif

Minh, you vote in favour of math-typeset numbers? (I don't like e.g. L-functions either...)

I'm not particularly picky about this issue. What you proposed in your patches are OK by me.

Well, I haven't reverted your changes of e.g. 1 to 1; the HTML output is not very nice...

I have folded all our reviewer patches into one cumulative patch.

Yes, of course noticed that.

I would prefer something like "boolean", "integer", "real number", etc. Something as "meaningful" as possible, without recourse to type information.

This might not sound pythonic, but I'd prefer documenting the concrete type if the function actually expects some Python type (rather than a duck).

We now need someone to review the technical (mathematical) aspect of John's patch,

rlm?

and a sign off on my review patch.

I've positively reviewed your changes... :)

So unfortunately someone else (other than John and us) has to review the cumulative reviewer patch to avoid mutual peer-reviewing on the same ticket (some people seem to have no problems with this, I do).

### comment:19 follow-ups:  20  21 Changed 13 years ago by cremona

Owner: changed from mvngu to cremona

For what it's worth, thanks again to both, and I am quite happy with the combined reviewer patch (trac_8799-reviewer-total.patch).

I did not see the typo in the code which Leif mentioned. Is it where we test height_bound > 21.4 and not 21.48? In that place, it is true that this could be approximately doubled on 64-bit machines (I guess that when this code was first written the person doing it did not know how to test that) but in practice using bounds > 21 will take a very very long time.

So we still need an independent reviewer... and some of the points raised on this ticket deserve a wider audience, possibly additional sentences in the developers guide?

### comment:20 in reply to:  19 Changed 13 years ago by leif

I did not see the typo in the code which Leif mentioned. Is it where we test height_bound > 21.4 and not 21.48?

No. I meant:

-        verbose == bool(verbose)
+        verbose = bool(verbose)


In that place, it is true that this could be approximately doubled on 64-bit machines (I guess that when this code was first written the person doing it did not know how to test that) but in practice using bounds > 21 will take a very very long time.

It's just that the implementation doesn't meet the documentation:

        - height_limit (float, default: 18) -- search up to this
logarithmic height.

.. note::

On 32-bit machines, this *must* be < 21.48 else
\exp(h_{\text{lim}}) > 2^{31} and overflows.  On 64-bit machines, it
must be *at most* 43.668.  However, this bound is a logarithmic
bound and increasing it by just 1 increases the running time
by (roughly) \exp(1.5)=4.5, so searching up to even 20
takes a very long time.


(We could add here that larger heights for 64-bit code currently aren't implemented in the Python interface.)

        height_limit = float(height_limit)
if height_limit >= 21.4:	# TODO: docstring says 21.48 (for 32-bit machines; what about 64-bit...?)
raise ValueError, "The height limit must be < 21.4."


Also, if the docstring says 21.48, the code should use the same value.

(Note that the code should check for what machine the code/mwrank was compiled for, not what machine Sage is currently running on...)

But that (change of the implementation, and perhaps other TODOs, like in mwrank_EllipticCurve.__repr__()) should be fixed on another ticket.

[...] some of the points raised on this ticket deserve a wider audience, possibly additional sentences in the developers guide?

Definitely. :)

-Leif

### comment:21 in reply to:  19 Changed 13 years ago by leif

[...] some of the points raised on this ticket deserve a wider audience, possibly additional sentences in the developers guide?

I've opened a ticket for that at http://trac.sagemath.org/sage_trac/ticket/8958.

### comment:22 follow-up:  23 Changed 13 years ago by cremona

Status: needs_review → needs_work

I just checked that the patches apply fine to 4.4.2.rc0 (they do) and did a full make ptest.

This reveals a small problem in sage/libs/mwrank.pyx: The reviewer patch removes two L suffixes from the output of E.discriminant() and E.conductor(), on long python ints. I think this must be 32/64-bit dependent. The best soution (surely) is to make those functions return Sage integers in the first place. I will add that.

In the mean time I have put this back to "need work", realising that it has missged the boat for 4.4.2 anyway...

### comment:23 in reply to:  22 ; follow-up:  24 Changed 13 years ago by leif

In the mean time I have put this back to "need work", realising that it has missged the boat for 4.4.2 anyway...

mvngu wrote:

From hereon, only critical fixes would be merged. Fixes for documentation could also be merged.

;-)

### comment:24 in reply to:  23 Changed 13 years ago by cremona

In the mean time I have put this back to "need work", realising that it has missged the boat for 4.4.2 anyway...

mvngu wrote:

From hereon, only critical fixes would be merged. Fixes for documentation could also be merged.

;-)

I know (and with that in mind I asked a couple of people of they would step in) but now I am suggesting a non-docstring change...

### comment:25 Changed 13 years ago by cremona

Description: modified (diff) needs_work → needs_review

The final patch trac_8799-extra.patch fixes the issues raised by returning python ints (which display differently on 32 and 64 bit machines): the return types are Sage Integers, which is better anyway.

### comment:26 Changed 13 years ago by davidloeffler

Status: needs_review → positive_review

The code looks fine, the patch applies and builds fine, the reference manual builds with no warnings, and all doctests pass. Positive review.

### comment:27 Changed 13 years ago by davidloeffler

Authors: John Cremona → John Cremona, Minh Van Nguyen, Leif Leonhardy Minh Van Nguyen, Leif Leonhardy → Minh Van Nguyen, Leif Leonhardy, David Loeffler

### comment:28 Changed 13 years ago by mhansen

Merged in: → sage-4.4.4.alpha0 → fixed positive_review → closed
Note: See TracTickets for help on using tickets.