Get p-adic doctest coverage to 100% (depends on #5105) #5778

roed314 · 2009-04-13T17:44:04Z

Note:

This patch depends on behaviour of the norm function in the p-adic ring #5105 and will not apply otherwise.
It only applies to 3.4.1 at the moment. There are some whitespace changes in 3.4.2.a0 that will need to be sorted out, but I (Michael) will take care of this even if the patches get a positive review against 3.4.1 only. If someone wants to rebase this is fine by me :)

Added p-adics to the reference manual, 100% doctest coverage, converts p-adics to new coercion model, good ReST compatibility for files up to partway through padic_ZZ_pX_FM_element.pyx (alphabetically), no docbuild errors for files later alphabetically.

Component: padics

Author: David Roe

Reviewer: Robert Bradshaw

Merged: 4.0.alpha0

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

aghitza · 2009-04-15T00:12:31Z

comment:2

Hi David,

This patch fails to apply to my 3.4.1.rc2:

applying padic_doctests.patch
patching file sage/rings/padics/factory.py
Hunk #1 FAILED at 291
Hunk #2 FAILED at 358
Hunk #3 FAILED at 483
Hunk #4 FAILED at 518
Hunk #5 FAILED at 529
Hunk #6 FAILED at 787
Hunk #7 FAILED at 805
Hunk #8 FAILED at 820
Hunk #9 FAILED at 829
Hunk #10 FAILED at 1141
Hunk #11 FAILED at 1207
Hunk #12 FAILED at 1679
Hunk #13 FAILED at 1705
Hunk #14 FAILED at 1714
Hunk #15 FAILED at 1723
Hunk #16 FAILED at 1732
Hunk #17 FAILED at 1744
Hunk #18 succeeded at 525 with fuzz 2 (offset -1295 lines).
Hunk #19 succeeded at 570 with fuzz 2 (offset -1300 lines).
Hunk #20 FAILED at 1879
Hunk #21 succeeded at 582 with fuzz 1 (offset -1307 lines).
18 out of 23 hunks FAILED -- saving rejects to file sage/rings/padics/factory.py.rej
patching file sage/rings/padics/padic_ZZ_pX_CA_element.pyx
Hunk #39 FAILED at 1625
1 out of 45 hunks FAILED -- saving rejects to file sage/rings/padics/padic_ZZ_pX_CA_element.pyx.rej
patching file sage/rings/padics/padic_printing.pyx
Hunk #1 FAILED at 19
Hunk #9 FAILED at 228
Hunk #10 FAILED at 303
Hunk #11 FAILED at 385
Hunk #13 FAILED at 519
Hunk #15 FAILED at 562
Hunk #16 succeeded at 556 with fuzz 2 (offset -107 lines).
Hunk #20 FAILED at 894
Hunk #21 FAILED at 923
Hunk #22 FAILED at 975
Hunk #23 succeeded at 797 with fuzz 1 (offset -197 lines).
9 out of 24 hunks FAILED -- saving rejects to file sage/rings/padics/padic_printing.pyx.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh padic_doctests.patch

I'll try to take a look and see what's going on, but it might take a while.

sagetrac-mabshoff · 2009-04-15T01:16:18Z

comment:3

This patch does not import particularly well:

mabshoff@sage:/scratch/mabshoff/sage-3.4.1.rc3/devel/sage$ hg import padic_doctests.patch 
applying padic_doctests.patch
patching file sage/rings/padics/factory.py
Hunk #1 FAILED at 291
Hunk #2 FAILED at 358
Hunk #3 FAILED at 483
Hunk #4 FAILED at 518
Hunk #5 FAILED at 529
Hunk #6 FAILED at 787
Hunk #7 FAILED at 805
Hunk #8 FAILED at 820
Hunk #9 FAILED at 829
Hunk #10 FAILED at 1141
Hunk #11 FAILED at 1207
Hunk #12 FAILED at 1679
Hunk #13 FAILED at 1705
Hunk #14 FAILED at 1714
Hunk #15 FAILED at 1723
Hunk #16 FAILED at 1732
Hunk #17 FAILED at 1744
Hunk #18 succeeded at 525 with fuzz 2 (offset -1295 lines).
Hunk #19 succeeded at 570 with fuzz 2 (offset -1300 lines).
Hunk #20 FAILED at 1879
Hunk #21 succeeded at 582 with fuzz 1 (offset -1307 lines).
18 out of 23 hunks FAILED -- saving rejects to file sage/rings/padics/factory.py.rej
patching file sage/rings/padics/padic_ZZ_pX_CA_element.pyx
Hunk #39 FAILED at 1625
1 out of 45 hunks FAILED -- saving rejects to file sage/rings/padics/padic_ZZ_pX_CA_element.pyx.rej
patching file sage/rings/padics/padic_printing.pyx
Hunk #1 FAILED at 19
Hunk #9 FAILED at 228
Hunk #10 FAILED at 303
Hunk #11 FAILED at 385
Hunk #13 FAILED at 519
Hunk #15 FAILED at 562
Hunk #16 succeeded at 556 with fuzz 2 (offset -107 lines).
Hunk #20 FAILED at 894
Hunk #21 FAILED at 923
Hunk #22 FAILED at 975
Hunk #23 succeeded at 797 with fuzz 1 (offset -197 lines).
9 out of 24 hunks FAILED -- saving rejects to file sage/rings/padics/padic_printing.pyx.rej
abort: patch failed to apply

I am not sure if there is a missing dependency.

David: any ideas?

Cheers,

Michael

sagetrac-mabshoff · 2009-04-15T02:33:29Z

comment:4

Oops, for whatever reason I did not have #4637 in my tree - so apologies.

Cheers,

Michael

sagetrac-mabshoff · 2009-04-15T02:41:17Z

comment:5

Ok, with this patch applied:

sage-3.4.1.rc3/devel/sage$ patch -p1 < padic_doctests.patch 
patching file sage/rings/padics/factory.py
patching file sage/rings/padics/padic_ZZ_pX_CA_element.pyx
patching file sage/rings/padics/padic_ZZ_pX_CR_element.pyx
patching file sage/rings/padics/padic_ZZ_pX_FM_element.pyx
patching file sage/rings/padics/padic_capped_absolute_element.pyx
patching file sage/rings/padics/padic_capped_relative_element.pxd
patching file sage/rings/padics/padic_capped_relative_element.pyx
patching file sage/rings/padics/padic_ext_element.pxd
patching file sage/rings/padics/padic_ext_element.pyx
patching file sage/rings/padics/padic_fixed_mod_element.pyx
patching file sage/rings/padics/padic_generic.py
patching file sage/rings/padics/padic_generic_element.pyx
patching file sage/rings/padics/padic_printing.pyx
patching file sage/rings/padics/pow_computer.pyx
patching file sage/rings/padics/pow_computer_ext.pyx

We also get:

Overall weighted coverage score:  38.2%
Total number of functions:  840
We need  250 more function to get to 68% coverage.
We need  267 more function to get to 70% coverage.
We need  309 more function to get to 75% coverage.

for the padics directory only.

Cheers,

Michael

sagetrac-mabshoff · 2009-04-15T02:49:46Z

comment:6

There are some slight doctesting failures:

The following tests failed:

sage -t -long devel/sage/sage/schemes/elliptic_curves/padics.py # 3 doctests failed
sage -t -long devel/sage/sage/schemes/elliptic_curves/padic_lseries.py # 2 doctests failed
sage -t -long devel/sage/sage/schemes/elliptic_curves/sha_tate.py # 2 doctests failed
sage -t -long devel/sage/sage/schemes/elliptic_curves/ell_tate_curve.py # 4 doctests failed
sage -t -long devel/sage/sage/rings/padics/factory.py # 2 doctests failed
sage -t -long devel/sage/sage/rings/padics/padic_capped_relative_element.pyx # 1 doctests failed

Cheers,

Micheal

sagetrac-mabshoff · 2009-04-15T06:58:32Z

comment:7

Bouncing it to 3.4.2 - if the patch is updated, passes doctests and is reviewed there might be a chance to get this into 3.4.1.

Cheers,

Michael

sagetrac-mabshoff · 2009-04-18T03:56:04Z

comment:9

Looks much better. One trivial hashing issue (32 vs. 64 bit) needs to be fixed:

sage -t -long "devel/sage/sage/rings/padics/padic_capped_relative_element.pyx"
**********************************************************************
File "/scratch/mabshoff/sage-3.4.1.rc4/devel/sage/sage/rings/padics/padic_capped_relative_element.pyx", line 2357:
    sage: hash(R(-1))
Expected:
    1977822444
Got:
    95367431640624
**********************************************************************

Cheers,

Michael

JohnCremona · 2009-04-18T16:22:16Z

comment:10

Michael, are you actually reviewing this or just making sure that it applies and passes tests before it can be reviewed? John

sagetrac-mabshoff · 2009-04-18T16:36:59Z

comment:11

Replying to @JohnCremona:

Hi John,

Michael, are you actually reviewing this or just making sure that it applies and passes tests before it can be reviewed? John

I am not reviewing this since I don't feel qualified, I just made sure that the patch applied and passes doctests.

Cheers,

Michael

JohnCremona · 2009-04-18T16:58:04Z

comment:12

Replying to @sagetrac-mabshoff:

I am not reviewing this since I don't feel qualified, I just made sure that the patch applied and passes doctests.

That's what I thought. I'll take a look. John

JohnCremona · 2009-04-18T20:19:44Z

comment:13

The patch applies and tests pass.

As I cannot view the patch (it is too big) I don't actually know what has changed. Apparently the files have been restified, but the only way to test that would be to add them to the reference manual and try building. I started with factory.py as that seemed pretty important. I found that the file while partially restified on a superficial level was completely broken as far as actually processing it is concerned. I started correcting it and got to line 430, but it is 2200 lines long and so will take a lot longer.

I suggest that this patch goes in since it is a step in the right direction; but I don't promise to even get to the end of properly getting the docs for factory.py into the manual, let alone any of the rest (ha ha ) of it.

On the positive side I not the very comprhjensive explanation of the p-adic code in a separate tutorial file which is in the reference manual; but I think that we do need the individual files (of which there are a large number) in there too.

sagetrac-mabshoff · 2009-04-18T22:07:36Z

comment:14

The patch is not ready to be build with ReST, David just started changing the doctests as a step in the right direction. About 2/3 of the patch are indentation changes, the other 1/3 adds new doctests. I have taken a look a the patch via a local diff viewer since as you point out the patch is too large for trac.

Given your reservations I would like someone else to take another look, so I am setting this to "needs review" again.

David is also working on subsequent patches to add more doctests and his eventual goal here is to get all of padics 100% tested and in the reference manual. It might be a good idea to stay below 256kb/patch in the future ;)

Cheers,

Michael

roed314 · 2009-04-22T10:27:41Z

comment:15

I'm adding a bunch more doctests and breaking this up into more managable chunks (viewable at least). If anyone wants to review this, let me know, but you probably don't want to get started quite yet.

roed314 · 2009-05-01T07:56:55Z

comment:35

Certainly.

capped_absolute_generic.py, capped_relative_field_generic.py, capped_relative_generic.py, capped_relative_ring_generic.py, fixed_mod_generic.py, padic_capped_absolute_ring_generic.py, padic_capped_relative_field_generic.py, padic_capped_relative_ring_generic.py, padic_field_base_generic.py, padic_field_generic.py, padic_fixed_mod_ring_generic.py, padic_ring_base_generic.py, padic_ring_generic.py ----> generic_nodes.py

padic_field_capped_relative.py, padic_ring_capped_absolute.py, padic_ring_capped_relative.py, padic_ring_fixed_mod.py ----> padic_base_leaves.py

lazy_field_generic.py, lazy_generic.py, lazy_ring_generic.py, padic_field_lazy.py, padic_lazy_element.py, padic_lazy_field_generic.py, padic_lazy_generic.py, padic_lazy_ring_generic.py, padic_ring_lazy.py, valuation.py ----> deleted because lazy p-adics not supported and won't be for a while

rigid_functions.pyx, rigid_functions.pxd ----> deleted because I didn't want to write the doctest and make parents.

robertwb · 2009-05-02T06:05:35Z

comment:36

First, the patches can't be partially applied and used one at a time. But it is better than one monolithic patch.

OK, the first two patches (outside-padics and deletions/moving look good). I'm all for a generic_nodes.py rather than a dozen files with three lines in them each (it makes it a lot easier to trace the code for instance).

I'm most of the way through padic_doctests_1.patch--it looks good for the most part. Lots of the patch is whitespace/line wrapping--it would be nice to be able to filter stuff like this out better for review purposes. There's a fair amount of commenting stuff out/ReSTification as well, and new doctests. The only issues I've found are

sage/rings/padics/eisenstein_extension_generic.py:97 - typo "extensinos"

sage/rings/padics/factory.py:2323: def krasner_check(poly, prec): always returns True, but comments state that it's really not implemented, which is a bit worrisome.

robertwb · 2009-05-03T08:02:09Z

comment:37

Some more comments:

padic_doctests_2.patch

looks good, again, mostly rest/whitespace changes.

padic_doctests_3.patch

sage/rings/padics/padic_ZZ_pX_element.pyx:74 extraneous print statements
sage/rings/padics/padic_base_generic.py::33 generic __reduce__ commented out, should probably be outright deleted if it's not used (do subclasses always override this?)
sage/rings/padics/padic_base_generic_element.pyx:43 (nitpicky) I think it's easier to read
```
"%s + O(%s^%s)" % (self.lift(), self.parent().prime(), self.precision_absolute())
```


than

    self.lift().str() + " + O(" + self.parent().prime().str() + "^" + self.precision_absolute().str() + ")"


Also, something I just noticed (not part of this patch). The functions `_set_to_mpz`, `_set_to_mpq` don't change self (as one would expect) but set the input parameter. Perhaps `_set_mpz_to` or something similar would be better.

robertwb · 2009-05-03T08:02:43Z

comment:38

sage/rings/padics/padic_capped_absolute_element.pyx:382

-            sage: R(7^5)._is_inexact_zero() 
+            sage: R(0,4)._is_inexact_zero()

The former is a better example, IMHO.

robertwb · 2009-05-03T08:09:40Z

comment:39

Some general remarks:

The AUTHORS block is not a verbatim/code block, no need for double colons.
Several docstrings are duplicated in their entirety, e.g. for a cdef function and its testing function. There really should just be one copy of the docstring, and the other should reference the first.
Some examples, e.g. for _div_ could be more illustrative (e.g. showing that div produces multiplicative inverses would be interesting, as would dividing by non-units).
I like that there is some variety in the primes used, including big ones, but I would like to see more variety in the extension fields used. The field Q_5(a) where a is a root of f = x^5 + 75*x^3 - 15*x^2 +125*x - 5 is used in nearly every example (i.e. over 150 times).

roed314 · 2009-05-04T09:47:42Z

comment:40

Things left to do:

Continue through the padic_ZZ_pX files and add more examples and tests using other extensions (started in padic_ZZ_pX_CR_element.pyx).
Continue improving ReST compliance in files after padic_capped_absolute_element.pyx

Robert, are there other files besides padic_ZZ_pX_element.pyx that you had in mind for the comment about docstrings being duplicated?

robertwb · 2009-05-06T05:57:53Z

comment:41

padic_ZZ_pX_element.pyx was the first place I noticed duplicated doctests, but it's a pattern I think I saw several times.

robertwb · 2009-05-07T03:24:01Z

comment:42

There's also some verbatim duplicated doctests in sage/rings/padics/padic_ext_element.pyx

There's no way the doctes for long valuation_c at sage/rings/padics/padic_generic.py:814 is actually being tested.

robertwb · 2009-05-07T06:32:44Z

comment:43

I've finished reading everything.

PowComputer_ext._pow_ZZ_tmp_demo is basically a verbatim copy of PowComputer_class._pow_ZZ_tmp_demo

pAdicZZpXFMElement._teichmuller_set and PowComputer_ZZ_pX.teichmuller_set_c have nearly the same exact doctest, as do pAdicCappedRelativeElement._to_gen and pAdicCappedRelativeElement._pari_. I can see why one would want to test it in both places, but it would be better to test with distinct elements. There's probably others, I just get a very deja-vu feeling reading some of these doctests...

How many more files need to be ReSTified? If its more than a one or two, lets defer doing this to a later ticket so we can focus on getting this in.

Your referee patch looks good too, and does address most of my concerns.

roed314 · 2009-05-07T06:49:51Z

comment:44

About 13 more files need improved ReSTification.

I'll try to diversify the doctests for those functions in a bit. Right now I'm working on p-adic polynomials though. :-)

robertwb · 2009-05-07T07:02:19Z

comment:45

robert$ ls sage/rings/padics/*.py* | wc
      30      30    1192

So nearly half of the files still need ReST conversion? Let's put this off to a later ticket, so we can get this one here into 4.0.

roed314 · 2009-05-07T07:05:21Z

comment:46

Yep, I agree. There are no build errors for any of them, but there are lots of files that need many ` added.

sagetrac-mabshoff · 2009-05-10T13:40:54Z

comment:47

With #5105 applied all patches apply and I am seeing one issue on sage.math:

sage -t -long "devel/sage/sage/rings/padics/padic_ZZ_pX_element.pyx"
**********************************************************************
File "/scratch/mabshoff/sage-4.0.alpha0/devel/sage/sage/rings/padics/padic_ZZ_pX_element.pyx", line 654:
    sage: _find_val_aprec_test(Zq(25,names='a'), [15, int(75), ntl_ZZ(625)])
Expected:
    (1, 340282366920938463463374607431768211457, 2)
Got:
    (4, 340282366920938463463374607431768211457, 2)
**********************************************************************
1 items had failures:

But padic_referee_fixes_2.patch introduces some problem since the latex() methods use mathbf() instead of ZZ or QQ for example.

Cheers,

Michael

roed314 · 2009-05-11T06:17:15Z

comment:48

Hah. On sage.math:

sage: from sage.libs.ntl.all import ZZ as ntl_ZZ
sage: ntl_ZZ(4) < 1
True
sage: ntl_ZZ(1) < 4
True

On my machine:

sage: from sage.libs.ntl.all import ZZ as ntl_ZZ
sage: ntl_ZZ(4) < 1
False
sage: ntl_ZZ(1) < 4
False

ntl_ZZ is just comparing types. I'll change the code to convert to Integers earlier.

Replying to @sagetrac-mabshoff:

With #5105 applied all patches apply and I am seeing one issue on sage.math:

sage -t -long "devel/sage/sage/rings/padics/padic_ZZ_pX_element.pyx"
**********************************************************************
File "/scratch/mabshoff/sage-4.0.alpha0/devel/sage/sage/rings/padics/padic_ZZ_pX_element.pyx", line 654:
    sage: _find_val_aprec_test(Zq(25,names='a'), [15, int(75), ntl_ZZ(625)])
Expected:
    (1, 340282366920938463463374607431768211457, 2)
Got:
    (4, 340282366920938463463374607431768211457, 2)
**********************************************************************
1 items had failures:

But padic_referee_fixes_2.patch introduces some problem since the latex() methods use mathbf() instead of ZZ or QQ for example.

Cheers,

Michael

roed314 · 2009-05-11T09:20:31Z

Attachment: padic_doctests_1.patch.gz

roed314 · 2009-05-11T09:20:43Z

Attachment: padic_doctests_2.patch.gz

Attachment: padic_doctests_3.patch.gz

roed314 · 2009-05-11T09:21:41Z

Attachment: padic_doctests_4.patch.gz

Attachment: padic_referee_fixes.patch.gz

roed314 · 2009-05-11T10:16:50Z

Attachment: padic_referee_fixes_2.patch.gz

roed314 · 2009-05-11T10:19:29Z

Attachment: padic_doctests_deletions.patch.gz

sagetrac-mabshoff · 2009-05-11T10:38:11Z

comment:49

Attachment: padic_doctests_outside.patch.gz

I am giving this ticket a positive review in RobertWB's name. It now passes all doctests on sage.math, it applies and builds, so any more concerns should be addressed via followup tickets. Post merge we are definitely in better shape than before and given the size of this patch it seems like a good idea to get this in. With all 8 patches applied:

Overall weighted coverage score:  74.4%
Total number of functions:  21967
We need  133 more function to get to 75% coverage.

sagetrac-mabshoff · 2009-05-11T10:38:53Z

comment:50

Merged

trac_5778_padic_doctests_1.patch
trac_5778_padic_doctests_2.patch
trac_5778_padic_doctests_3.patch
trac_5778_padic_doctests_4.patch
trac_5778_padic_doctests_deletions.patch
trac_5778_padic_doctests_outside.patch
trac_5778_padic_referee_fixes_2.patch
trac_5778_padic_referee_fixes.patch

in Sage 4.0.alpha0.

Cheers,

Michael

robertwb · 2009-05-11T19:24:49Z

comment:51

To followup now that I've had a chance to look at the last patch, yet, positive review deserved.

loefflerd · 2009-06-09T08:33:30Z

Reviewer: Robert Bradshaw

loefflerd · 2009-06-09T08:33:30Z

Author: David Roe

loefflerd · 2009-06-09T08:33:30Z

Merged: 4.0.alpha0

roed314 added this to the sage-3.4.1 milestone Apr 13, 2009

roed314 added c: number theory labels Apr 13, 2009

roed314 assigned williamstein Apr 13, 2009

roed314 changed the title ~~More p-adic doctests~~ More p-adic doctests [with patch; needs review] Apr 13, 2009

sagetrac-mabshoff mannequin changed the title ~~More p-adic doctests [with patch; needs review]~~ More p-adic doctests Apr 15, 2009

sagetrac-mabshoff mannequin added the s: needs review label Apr 15, 2009

sagetrac-mabshoff mannequin added s: needs work and removed s: needs review labels Apr 15, 2009

sagetrac-mabshoff mannequin modified the milestones: sage-3.4.1, sage-3.4.2 Apr 15, 2009

roed314 added s: needs review and removed s: needs work labels Apr 17, 2009

JohnCremona changed the title ~~More p-adic doctests~~ [with positive review but serious reservations] More p-adic doctests Apr 18, 2009

sagetrac-mabshoff mannequin changed the title ~~[with positive review but serious reservations] More p-adic doctests~~ More p-adic doctests Apr 18, 2009

sagetrac-mabshoff mannequin added s: positive review and removed s: needs review labels May 11, 2009

sagetrac-mabshoff mannequin removed the s: positive review label May 11, 2009

sagetrac-mabshoff mannequin closed this as completed May 11, 2009

sagetrac-mabshoff mannequin mentioned this issue May 11, 2009

behaviour of the norm function in the p-adic ring #5105

Closed

Get p-adic doctest coverage to 100% (depends on #5105) #5778

Get p-adic doctest coverage to 100% (depends on #5105) #5778

Comments

roed314 commented Apr 13, 2009

aghitza commented Apr 15, 2009

sagetrac-mabshoff mannequin commented Apr 15, 2009

sagetrac-mabshoff mannequin commented Apr 15, 2009

sagetrac-mabshoff mannequin commented Apr 15, 2009

sagetrac-mabshoff mannequin commented Apr 15, 2009

sagetrac-mabshoff mannequin commented Apr 15, 2009

sagetrac-mabshoff mannequin commented Apr 18, 2009

JohnCremona commented Apr 18, 2009

sagetrac-mabshoff mannequin commented Apr 18, 2009

JohnCremona commented Apr 18, 2009

JohnCremona commented Apr 18, 2009

sagetrac-mabshoff mannequin commented Apr 18, 2009

roed314 commented Apr 22, 2009

roed314 commented May 1, 2009

robertwb commented May 2, 2009

robertwb commented May 3, 2009

robertwb commented May 3, 2009

robertwb commented May 3, 2009

roed314 commented May 4, 2009

robertwb commented May 6, 2009

robertwb commented May 7, 2009

robertwb commented May 7, 2009

roed314 commented May 7, 2009

robertwb commented May 7, 2009

roed314 commented May 7, 2009

sagetrac-mabshoff mannequin commented May 10, 2009

roed314 commented May 11, 2009

roed314 commented May 11, 2009

roed314 commented May 11, 2009

roed314 commented May 11, 2009

roed314 commented May 11, 2009

roed314 commented May 11, 2009

sagetrac-mabshoff mannequin commented May 11, 2009

sagetrac-mabshoff mannequin commented May 11, 2009

robertwb commented May 11, 2009

loefflerd mannequin commented Jun 9, 2009

loefflerd mannequin commented Jun 9, 2009

loefflerd mannequin commented Jun 9, 2009