Reimplement elements of Zp and Qp using templates

[roed]

The goal of this ticket is to reimplement elements of Zp and Qp using the ideas from sage/rings/polynomial/polynomial_template.pxi. This should make p-adics in Sage both easier to maintain and easier to add new classes.

The final patch should not affect the external interface to Zp and Qp.

---

Apply

• trac_12555.patch​
• trac_12555_review.patch​
• 12555_rebase_6223.patch​
• trac_12555_doc_improvements.patch​
• trac_12555-pdf_fix-ts.patch​

[sydahmad]

Replying to roed:

The goal of this ticket is to reimplement elements of Zp and Qp using the ideas from sage/rings/polynomial/polynomial_template.pxi. This should make p-adics in Sage both easier to maintain and easier to add new classes.

[roed]

Now includes capped absolute and fixed modulus elements. There are a few problems remaining:

• teichmuller_list is disabled since it had a bug and I didn't want to fix it at the time. I'll track this down later.
• Some coercions aren't working since the element class has changed.

This version should be enough for Justin to compare the performance of the templated and non-templated versions. I'm now going to create another patch on top of this which removes the untemplated versions.

[roed]

Still some bugs, but it should be useable to compare timings

[roed]

I've fixed all known bugs; currently doctesting

[was]

this is my REFEREE patch after reading through half the code. I stopped at padic_ZZ_pX_FM_element.pyx. This is the ultimate patch bomb. Very hard to referee...

[saraedum]

A review of this patch is at https://github.com/saraedum/padic-review/commit. We agreed to work on the remaining issues there.

[roed]

I've made a pull request to https://github.com/saraedum/padic-review/commit, addressing the comments. Tests all pass for me.

We had discussed having a document that runs some overall tests. I haven't made that yet, but can this week. If someone has time to take a look at this this week, I'll be available to make changes....

[saraedum]

Replying to roed:

I've made a pull request to https://github.com/saraedum/padic-review/commit, addressing the comments. Tests all pass for me.

Great.

We had discussed having a document that runs some overall tests. I haven't made that yet, but can this week. If someone has time to take a look at this this week, I'll be available to make changes....

You haven't started to work on this already, have you? I just started to work on these tests. I'll push to github as soon as I have something working.

[roed]

We had discussed having a document that runs some overall tests. I haven't made that yet, but can this week. If someone has time to take a look at this this week, I'll be available to make changes....

You haven't started to work on this already, have you? I just started to work on these tests. I'll push to github as soon as I have something working.

No, I haven't started. I'll wait to see what you come up with.

Do we have a list of things that need to be done before this gets a positive review?

[saraedum]

Replying to roed:

Do we have a list of things that need to be done before this gets a positive review?

Not yet. Should we use github issues for this?

[roed]

Replying to saraedum:

Replying to roed:

Do we have a list of things that need to be done before this gets a positive review?

Not yet. Should we use github issues for this?

Sure, that sounds good.

[saraedum]

apply trac_12555.patch

[saraedum]

apply trac_12555.patch

[saraedum]

apply trac_12555.patch

[saraedum]

apply trac_12555.patch

[saraedum]

apply trac_12555.patch

[saraedum]

review patch to match changes done in 5.9.beta0

[jdemeyer]

Please rebase to #6223.

[jdemeyer]

This still needs work rebasing to #6223. Also PyErr_Occured() should not be used anymore in favour of standard Python except handling.

[jdemeyer]

12555_rebase_6223.patch​ needs review.

[roed]

Those changes look fine to me.

[jdemeyer]

Please fix this documentation problem (I won't):

[139 [690]] [130
! Missing $ inserted.
<inserted text> 
                $
l.10592 $i^{\mbox{prec_cap}
                           }$ if the element was defined by
? ]
! Emergency stop.
<inserted text> 
                $
l.10592 $i^{\mbox{prec_cap}
                           }$ if the element was defined by
!  ==> Fatal error occurred, no output PDF file produced!
Transcript written on padics.log.
 [17make[1]: *** [padics.pdf] Error 1
make[1]: Leaving directory `/mazur/release/merger/sage-5.10.beta1/devel/sage-main/doc/output/latex/en/reference/padics'

[jdemeyer]

* ping *

[roed]

Thanks for the ping. I had a visitor this week, but he just left so I'll take a look.

[roed]

combined github patch

[roed]

Sorry for the long delay: I've been mostly working on research rather than Sage this summer.

I was unable to reproduce the documentation problem that you observed by running sage -docbuild reference html. Is there something I should do instead, or somewhere with a more complete log of the problem? I'm not quite sure what needs to be fixed.

I did go through and change most of the dollar signs in the p-adics directory to backticks, and updated these patches against Sage 5.10.

[tscrim]

I think Jeroen was refering to the pdf doc:

sage -docbuild all pdf

maybe?

For patchbot:

Apply: trac_12555.patch trac_12555_review.patch 12555_rebase_6223.patch trac_12555_doc_improvements.patch

[roed]

Replying to tscrim:

I think Jeroen was refering to the pdf doc:

sage -docbuild all pdf

maybe?

Cool. I will try that overnight.

[tscrim]

Okay, I fixed the pdf docbuild issue and made some minor tweaks to that file.

Build finished.  The built documents can be found in /home/travis/sage-5.11.rc0/devel/sage/doc/output/pdf/en/reference/padics

If you're happy with my tweaks, we can set this (back to) positive review.

For patchbot:

Apply: trac_12555.patch, trac_12555_review.patch, 12555_rebase_6223.patch, trac_12555_doc_improvements.patch, trac_12555-pdf_fix-ts.patch​

[roed]

Thanks! I've been trying to focus on writing papers this summer rather than Sage. I'm happy with the changes.

[tscrim]

I'm interpreting that as a positive review.

[jdemeyer]

Needs to be rebased to sage-5.12.beta3.

[jpflori]

It seems to me this does not apply cleanly anymore to what I got when I clone Sage's git branch "build_system" as of today. I've pushed a hopefully correctly rebased branch to:

• u/jpflori/12555

Best, JP

[ohanar]

New commits:

​4b633ab	Fixes for some missing/duplicated chunks.
​66e5746	From Trac patches: trac_12555-pdf_fix-ts.patch.
​4e209b8	From Trac patches: trac_12555_doc_improvements.patch​.
​69d30a4	From Trac patches: 12555_rebase_6223.patch.
​11ad661	From Trac patches: trac_12555_review.patch.
​bd94fe3	From Trac patches: trac_12555.patch.
​a1f9bec	Merge remote-tracking branch 'origin/build_system' into build_system
​4cb7db9	Merge remote-tracking branch 'origin' into build_system

[vbraun]

Conflicts with current release - git merge master & resolve

[tscrim]

I've uploaded a branch with the merge. I'm recompiling now and will test it shortly.

---

New commits:

​7d7ff1f

Merge branch 'master' into public/padics/templates-12555

[git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

​0e7c964

Fixed failing doctest (likely due to #15422).

[vbraun]

Conflicts with sage-6.1.beta1, please fix

[tscrim]

I'm not familiar enough with the code to do this merge. David, Julian, could one of you do the merge?

[jpflori]

The problem is with commit fcf7972 "adapt miscellaneous files in the Sage library" which deals with Pari stuff. Note sure from which trac ticket this is: <hint hint> put trac ticket number into git commit messages... I'll fix it later today (currently my touchpad does not click, I cannot reboot, and tabbing all over the place is not so nice).

[jdemeyer]

Replying to jpflori:

Note sure from which trac ticket this is

#15185

<hint hint> put trac ticket number into git commit messages...

If you have to ask people, that's not going to work. When I started as release manager, I asked that and people forgot, were lazy, put the wrong number... or worse: got angry because I was complaining over such trivial things like ticket numbers in commit messages.

I learned that the only feasible solution is to have a system which automatically added the ticket number to the commit message. With git that cannot be done (that would be rewriting history), but at least automating the mapping commit -> ticket number could be done some other way.

[vbraun]

Don't put trac ticket numbers in the commit message, its useless double book-keeping. Its trivial to extract from the commit tree, e.g. using my "git trac" subcommand scripts it:

$ git trac find fcf7972
Commit has been merged by the release manager into your current branch.
commit c04a8de608cd41ed9d7ffbe250656bce03683301
Merge: 69b08fa 7ec74e0
Author: Release Manager <release@sagemath.org>
Date:   Fri Dec 20 13:21:14 2013 +0000

    Trac #15185: Clean up interface to the PARI library
    
    The file `sage/libs/pari/gen.pyx` is too big and contains too many
    different things.  For clarity and maintainability, it should be split
    into two parts:
    - `gen.pyx` containing the class `gen`;
    - `pari_instance.pyx` containing the class `PariInstance` and some
    utility functions;
    More cleaning-up will be done in #15461.
    
    URL: http://trac.sagemath.org/15185
    Reported by: pbruin
    Ticket author(s): Peter Bruin
    Reviewer(s): Jeroen Demeyer

[jdemeyer]

jdemeyer@tamiyo:/usr/local/src/sage-git$ git trac find fcf7972
git: 'trac' is not a git command. See 'git --help'.

Did you mean this?
        branch

[jdemeyer]

Rebasing to #15185 should be easy, it doesn't make any fundamental changes, only moves stuff around...

[vbraun]

No, I mean this: ​https://github.com/sagemath/git-trac-command

[jdemeyer]

vbraun: Are there any plans to make that script part of Sage?

[vbraun]

I think they are now ready to be used by a wider audience. Will make a package...

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​5f00813	Fixes for "sage not defined".
​89ef12d	Merge remote-tracking branch 'origin/develop' into ticket/12555

[jpflori]

Still some errors in src/sage/schemes/elliptic_curves/constructor.py and ell_egros.py raising:

SystemError: error return without exception set

when computing padic elliptic logs. Not sure where this comes from, maybe related to #15299.

[jpflori]

Apparently its when testing

y1 == -y2 - a1*x2 - a3

in ell_point.py. The following reproduces it:

sage: y1 = 10*11^2 + 3*11^3 + O(11^40)
sage: y2 = 11^2 + 7*11^3 + 10*11^4 + 10*11^5 + 10*11^6 + 10*11^7 + 10*11^8 + 10*11^9 + 10*11^10 + 10*11^11 + 10*11^12 + 10*11^13 + 10*11^14 + 10*11^15 + 10*11^16 + 10*11^17 + 10*11^18 + 10*11^19 + 10*11^20 + 10*11^21 + 10*11^22 + 10*11^23 + 10*11^24 + 10*11^25 + 10*11^26 + 10*11^27 + 10*11^28 + 10*11^29 + 10*11^30 + 10*11^31 + 10*11^32 + 10*11^33 + 10*11^34 + 10*11^35 + 10*11^36 + 10*11^37 + 10*11^38 + 10*11^39 + O(11^40)
sage: y1 == y2
---------------------------------------------------------------------------
SystemError                               Traceback (most recent call last)
<ipython-input-4-2a7abc374794> in <module>()
----> 1 y1 == y2

SystemError: error return without exception set
sage: 

[jpflori]

Ok it seems its just that mpz_cmp decides to return -2 which is also set as the except value.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​fcf6ad2

Fix for comparison of padics.

[jpflori]

Should be ok now. May be worth putting an additional test in there, but I'm too tired now and the code is already tested by the previous bug.

[tscrim]

That example (and testing the files) now works for me, so it's back to positive review.

EDIT - also I don't think .pxi files are run as part of doctesting?

[vbraun]

Documentation doesn't build

[padics   ] /mnt/SSD1/mod_buildslave/sage_git/build/src/doc/en/reference/padics/sage/rings/padics/padic_base_generic_element.rst:11: WARNING: autodoc can't import/find module 'sage.rings.padics.padic_base_generic_element', it reported error: "No module named padic_base_generic_element", please check your spelling and sys.path
[padics   ] docstring of sage.rings.padics.padic_fixed_mod_element:9: ERROR: Unknown directive type "NOTES".

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​f338b7f

Fix wrong NOTE block.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​aa65f59

Removed removed file from doc.

[tscrim]

New commits:

​aa65f59

Removed removed file from doc.

[niles]

There are a number of other power series bugs that have been waiting in one way or another for rewriting of padics. Could someone here take a look and comment on the appropriate tickets? They do not seem to be resolved by this one.

#9457: Buggy equality of power series -- The fix seems straightforward, but causes a bizarre doctest failure in sha_tate.py

#4656: Power series over p-adics treat inexact zeros inconsistently -- The fix here causes the same doctest to fail.

#8198: p-adic precision is dropped when matrix acts on vector -- Possibly a root cause of the previous two bugs.

#5075: Polynomials over inexact rings truncate inexact leading zeroes -- This was somehow fixed for polynomials over p-adics, but polynomials over other power series still have the issue. David Roe commented that it may be fixed as part of a larger patch (I hope there's not another one after this!)

[jdemeyer]

See #15653 for a blocker follow-up.