#12555 closed enhancement (fixed)
Reimplement elements of Zp and Qp using templates
Reported by:  roed  Owned by:  roed 

Priority:  major  Milestone:  sage6.1 
Component:  padics  Keywords:  sd53 padics templates 
Cc:  sydahmad, justin, wstein, JStarx, jpflori  Merged in:  
Authors:  David Roe, Julian Rueth  Reviewers:  William Stein, Soroosh Yazdani, Julian Rueth, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  public/padics/templates12555 (Commits)  Commit:  aa65f59d6fefe38a06f7577b0d98906242882fd0 
Dependencies:  #12625, #13299, #14287, #12575, #6223  Stopgaps: 
Description (last modified by )
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 padics 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
Attachments (17)
Change History (105)
comment:1 in reply to: ↑ description Changed 8 years ago by
 Cc sydahmad added
comment:2 Changed 8 years ago by
 Cc justin added
comment:3 Changed 8 years ago by
 Description modified (diff)
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 nontemplated versions. I'm now going to create another patch on top of this which removes the untemplated versions.
Changed 8 years ago by
Changed 8 years ago by
Changed 8 years ago by
Changed 8 years ago by
Changed 8 years ago by
Changed 8 years ago by
Changed 8 years ago by
Changed 8 years ago by
Changed 8 years ago by
comment:4 Changed 8 years ago by
 Cc wstein JStarx added
 Status changed from new to needs_review
comment:5 Changed 8 years ago by
 Dependencies set to #12625, #12575
Changed 8 years ago by
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...
comment:6 Changed 7 years ago by
 Reviewers set to William Stein, Soroosh Yazdani, Julian Rueth
 Status changed from needs_review to needs_work
A review of this patch is at https://github.com/saraedum/padicreview/commit
. We agreed to work on the remaining issues there.
comment:7 Changed 7 years ago by
 Dependencies changed from #12625, #12575 to #12625, #12575, #14106
comment:8 followup: ↓ 9 Changed 7 years ago by
 Milestone changed from sage5.8 to sage5.7
I've made a pull request to https://github.com/saraedum/padicreview/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....
comment:9 in reply to: ↑ 8 ; followup: ↓ 10 Changed 7 years ago by
Replying to roed:
I've made a pull request to
https://github.com/saraedum/padicreview/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.
comment:10 in reply to: ↑ 9 ; followup: ↓ 11 Changed 7 years ago by
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?
comment:11 in reply to: ↑ 10 ; followup: ↓ 12 Changed 7 years ago by
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?
comment:12 in reply to: ↑ 11 Changed 7 years ago by
comment:13 Changed 7 years ago by
 Dependencies changed from #12625, #12575, #14106 to #12625, #12575, #14106, #14285
comment:14 Changed 7 years ago by
apply trac_12555.patch
comment:15 Changed 7 years ago by
 Description modified (diff)
comment:16 Changed 7 years ago by
 Dependencies changed from #12625, #12575, #14106, #14285 to #12625, #12575, #14106, #14284
comment:17 Changed 7 years ago by
 Dependencies changed from #12625, #12575, #14106, #14284 to #12625, #12575, #14106, #14284, #14287
comment:18 Changed 7 years ago by
 Dependencies changed from #12625, #12575, #14106, #14284, #14287 to #12625, #12575, #13299, #14284, #14287
comment:19 Changed 7 years ago by
 Dependencies changed from #12625, #12575, #13299, #14284, #14287 to #12625, #12575, #13299, #14287
comment:20 Changed 7 years ago by
apply trac_12555.patch
comment:21 Changed 7 years ago by
 Dependencies changed from #12625, #12575, #13299, #14287 to #12625, #13299, #14287, #12575
comment:22 Changed 7 years ago by
 Status changed from needs_work to needs_review
comment:23 Changed 7 years ago by
apply trac_12555.patch
comment:24 Changed 7 years ago by
apply trac_12555.patch
comment:25 Changed 7 years ago by
 Status changed from needs_review to positive_review
comment:26 Changed 7 years ago by
apply trac_12555.patch
comment:27 Changed 7 years ago by
 Milestone changed from sage5.7 to sage5.9
comment:28 Changed 7 years ago by
 Description modified (diff)
 Milestone changed from sage5.9 to sage5.7
comment:29 Changed 7 years ago by
 Milestone changed from sage5.7 to sage5.9
comment:30 Changed 7 years ago by
 Milestone changed from sage5.9 to sagepending
comment:31 Changed 7 years ago by
 Milestone changed from sagepending to sage5.10
comment:32 Changed 7 years ago by
 Dependencies changed from #12625, #13299, #14287, #12575 to #12625, #13299, #14287, #12575, #6223
 Status changed from positive_review to needs_work
Please rebase to #6223.
comment:33 Changed 7 years ago by
 Status changed from needs_work to positive_review
comment:34 Changed 7 years ago by
 Status changed from positive_review to needs_work
This still needs work rebasing to #6223. Also PyErr_Occured()
should not be used anymore in favour of standard Python except
handling.
comment:35 Changed 7 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
comment:36 Changed 7 years ago by
12555_rebase_6223.patch needs review.
comment:37 Changed 7 years ago by
 Cc jpflori added
comment:38 Changed 7 years ago by
 Status changed from needs_review to positive_review
Those changes look fine to me.
comment:39 Changed 7 years ago by
 Status changed from positive_review to needs_work
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/sage5.10.beta1/devel/sagemain/doc/output/latex/en/reference/padics'
comment:40 Changed 7 years ago by
* ping *
comment:41 Changed 7 years ago by
Thanks for the ping. I had a visitor this week, but he just left so I'll take a look.
Changed 7 years ago by
Changed 7 years ago by
comment:42 Changed 7 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
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 padics directory to backticks, and updated these patches against Sage 5.10.
comment:43 followup: ↓ 44 Changed 7 years ago by
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
comment:44 in reply to: ↑ 43 Changed 7 years ago by
Replying to tscrim:
I think Jeroen was refering to the pdf doc:
sage docbuild all pdfmaybe?
Cool. I will try that overnight.
comment:45 Changed 7 years ago by
 Milestone changed from sage5.11 to sage5.12
Changed 7 years ago by
comment:46 Changed 7 years ago by
 Description modified (diff)
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/sage5.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_12555pdf_fixts.patch
comment:47 Changed 7 years ago by
Thanks! I've been trying to focus on writing papers this summer rather than Sage. I'm happy with the changes.
comment:48 Changed 7 years ago by
 Reviewers changed from William Stein, Soroosh Yazdani, Julian Rueth to William Stein, Soroosh Yazdani, Julian Rueth, Travis Scrimshaw
 Status changed from needs_review to positive_review
I'm interpreting that as a positive review.
comment:49 Changed 7 years ago by
 Status changed from positive_review to needs_work
Needs to be rebased to sage5.12.beta3.
comment:50 Changed 7 years ago by
 Branch set to u/saraedum/ticket/12555
 Modified changed from 08/25/13 09:32:42 to 08/25/13 09:32:42
comment:51 Changed 7 years ago by
 Description modified (diff)
 Milestone changed from sage5.12 to sage6.0
 Status changed from needs_work to positive_review
comment:52 Changed 7 years ago by
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
comment:53 Changed 7 years ago by
 Keywords sd53 padics templates added
comment:54 Changed 7 years ago by
 Branch changed from u/saraedum/ticket/12555 to u/jpflori/12555
comment:55 Changed 7 years ago by
 Branch changed from u/jpflori/12555 to u/jpflori/ticket/12555
comment:56 Changed 6 years ago by
 Commit set to 4b633ab664fd5bd062d29fcec78972232cde634c
New commits:
4b633ab  Fixes for some missing/duplicated chunks. 
66e5746  From Trac patches: trac_12555pdf_fixts.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 remotetracking branch 'origin/build_system' into build_system 
4cb7db9  Merge remotetracking branch 'origin' into build_system 
comment:57 Changed 6 years ago by
 Milestone changed from sage6.0 to sage6.1
comment:58 Changed 6 years ago by
Conflicts with current release  git merge master & resolve
comment:59 Changed 6 years ago by
 Branch changed from u/jpflori/ticket/12555 to public/padics/templates12555
 Commit changed from 4b633ab664fd5bd062d29fcec78972232cde634c to 7d7ff1fe557a360b590f8072f6da1abb9460b39e
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/templates12555 
comment:60 Changed 6 years ago by
 Commit changed from 7d7ff1fe557a360b590f8072f6da1abb9460b39e to 0e7c96482eb0d2ee036906c79ecbc122f3650eda
 Status changed from positive_review to needs_review
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).

comment:61 Changed 6 years ago by
 Status changed from needs_review to positive_review
comment:62 Changed 6 years ago by
Conflicts with sage6.1.beta1, please fix
comment:63 Changed 6 years ago by
I'm not familiar enough with the code to do this merge. David, Julian, could one of you do the merge?
comment:64 followup: ↓ 65 Changed 6 years ago by
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).
comment:65 in reply to: ↑ 64 Changed 6 years ago by
Replying to jpflori:
Note sure from which trac ticket this is
<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.
comment:66 Changed 6 years ago by
Don't put trac ticket numbers in the commit message, its useless double bookkeeping. 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 cleaningup will be done in #15461. URL: http://trac.sagemath.org/15185 Reported by: pbruin Ticket author(s): Peter Bruin Reviewer(s): Jeroen Demeyer
comment:67 Changed 6 years ago by
jdemeyer@tamiyo:/usr/local/src/sagegit$ git trac find fcf7972 git: 'trac' is not a git command. See 'git help'. Did you mean this? branch
comment:68 Changed 6 years ago by
Rebasing to #15185 should be easy, it doesn't make any fundamental changes, only moves stuff around...
comment:69 Changed 6 years ago by
No, I mean this: https://github.com/sagemath/gittraccommand
comment:70 Changed 6 years ago by
vbraun: Are there any plans to make that script part of Sage?
comment:71 Changed 6 years ago by
I think they are now ready to be used by a wider audience. Will make a package...
comment:72 Changed 6 years ago by
 Status changed from positive_review to needs_work
comment:73 Changed 6 years ago by
 Commit changed from 0e7c96482eb0d2ee036906c79ecbc122f3650eda to 5f0081329eb8dd9f843cf66c9f73a99d045ff53a
comment:74 Changed 6 years ago by
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.
comment:75 Changed 6 years ago by
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) <ipythoninput42a7abc374794> in <module>() > 1 y1 == y2 SystemError: error return without exception set sage:
comment:76 Changed 6 years ago by
Ok it seems its just that mpz_cmp decides to return 2 which is also set as the except value.
comment:77 Changed 6 years ago by
 Commit changed from 5f0081329eb8dd9f843cf66c9f73a99d045ff53a to fcf6ad2787c40314bd51953d21967af4a6c70297
Branch pushed to git repo; I updated commit sha1. New commits:
fcf6ad2  Fix for comparison of padics.

comment:78 Changed 6 years ago by
 Status changed from needs_work to needs_review
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.
comment:79 Changed 6 years ago by
 Status changed from needs_review to positive_review
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?
comment:80 Changed 6 years ago by
 Resolution set to fixed
 Status changed from positive_review to closed
comment:81 Changed 6 years ago by
 Resolution fixed deleted
 Status changed from closed to new
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".
comment:82 Changed 6 years ago by
 Commit changed from fcf6ad2787c40314bd51953d21967af4a6c70297 to f338b7fa5d11807a7c8dcde2ce745858e6cf047c
Branch pushed to git repo; I updated commit sha1. New commits:
f338b7f  Fix wrong NOTE block.

comment:83 Changed 6 years ago by
 Commit changed from f338b7fa5d11807a7c8dcde2ce745858e6cf047c to aa65f59d6fefe38a06f7577b0d98906242882fd0
Branch pushed to git repo; I updated commit sha1. New commits:
aa65f59  Removed removed file from doc.

comment:84 Changed 6 years ago by
 Status changed from new to needs_review
New commits:
aa65f59  Removed removed file from doc.

comment:85 Changed 6 years ago by
 Status changed from needs_review to positive_review
comment:86 Changed 6 years ago by
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 padics treat inexact zeros inconsistently  The fix here causes the same doctest to fail.
#8198: padic 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 padics, 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!)
comment:87 Changed 6 years ago by
 Resolution set to fixed
 Status changed from positive_review to closed
comment:88 Changed 6 years ago by
See #15653 for a blocker followup.
Replying to roed: