Opened 15 years ago
Closed 6 years ago
#812 closed enhancement (fixed)
add Pollack/Stevens overconvergent modular symbols code
Reported by:  Craig Citro  Owned by:  Marc Masdeu 

Priority:  major  Milestone:  sage7.3 
Component:  modular forms  Keywords:  padic Lfunctions 
Cc:  David Loeffler, David Roe, Craig Citro, wuthrich  Merged in:  
Authors:  Marc Masdeu, David Roe  Reviewers:  Chris Wuthrich 
Report Upstream:  N/A  Work issues:  
Branch:  44c321f (Commits, GitHub, GitLab)  Commit:  44c321f58f42ec0b11fbe8674545687b3562e73a 
Dependencies:  Stopgaps: 
Description (last modified by )
I'm just starting to work on implementing Pollack & Stevens' methods for using overconvergent modular symbols for padic Lfunctions, StarkHeegner points, etc.
Change History (104)
comment:1 Changed 15 years ago by
Status:  new → assigned 

comment:2 Changed 15 years ago by
Cc:  craigcitro@… added 

comment:3 Changed 14 years ago by
comment:6 Changed 11 years ago by
Report Upstream:  → N/A 

From Jennifer Balakrishnan:
Rob Pollack just ported over some of his padic Lseries via overconvergent modular symbols code to Sage, which could be very useful for our padic BSD paper.
The code he sent me originally didn't quite produce results matching our data, but I've worked on it over the last few days, fixed a few bugs, noticed a few more, and thought this might be a good coding sprint project for us this week.
Here's where the code currently stands:  it can compute the padic Lseries in the split p case (with data matching what we've already computed naively)  Rob says inert p is straightforward, just a matter of knowing how to call the right objects in Sage, which I think I can do  I've tested it against the regulators I computed in my thesis, and we can easily produce 10+ digits of precision in the Lvalue!!  for some primes, it results in memory errors (I've put examples, working and not, in the docstring in the test file). I'm not sure how to fix these.
As an enhancement, maybe we could also use some of your very fast code for modular symbols?
Perhaps most mathematically interesting, the special values computed by his program also result in the same +/1 "sign" in the BSD formula that our previous methods produced!
The code is available here:
http://sage.math.washington.edu/home/jen/OMS
To run it, attach master.sage and Jen/test_run_generic.sage. The second file (http://sage.math.washington.edu/home/jen/OMS/Jen/test_run_generic.sage) has some examples in the docstring.
Jen
comment:7 Changed 10 years ago by
Description:  modified (diff) 

comment:8 Changed 9 years ago by
Branch:  → u/mmasdeu/moved_from_githubfixed_several_bugs 

Commit:  → 9acf3ac74066be652fe7d28801c577d48a4f387d 
Owner:  changed from Craig Citro to Marc Masdeu 
New commits:
1b85377  Initial commit. Cloned from experimental branch of roed314's github.

1656711  Made all tests to pass.

72923ab  Increased coverage.

caaa672  Fixed precision of p_stabilize. Two more tests pass.

c0331a5  Fixed one test.

9fd7d69  All files pass the doctests except padic_lseries.py

4bbe597  All doctests passed, have 100% coverage.

c5aa41d  Removed ocmodule.py

ce76f3c  Removed references to old OCVn.

9acf3ac  Fixed one doctest.

comment:9 Changed 9 years ago by
Cc:  David Loeffler David Roe Craig Citro added; craigcitro@… removed 

Status:  new → needs_review 
comment:10 Changed 9 years ago by
I think the code is usable enough that it should made into Sage. It includes the btquotients code, which is in a quite stable state as well. Several parts of the code are in need for more debugging, though. Especially the dist.pyx and distributions.py, which are not very robust.
comment:11 Changed 9 years ago by
Status:  needs_review → needs_work 

There are some failing doctests, see patchbot's report
comment:12 Changed 9 years ago by
Commit:  9acf3ac74066be652fe7d28801c577d48a4f387d → a875b16d34b5e1a3ddf806b19770fa967b104443 

Branch pushed to git repo; I updated commit sha1. New commits:
a875b16  Fixed a doctest which failed when run without magma and long time flag enabled.

comment:13 Changed 9 years ago by
OK, I just learned that: # long time optional  magma does not work. One has to do: # long time, optional  magma
So hopefully now it works!
comment:14 Changed 9 years ago by
I just (very) quickly skimmed the patches  this is some beautiful code!
comment:15 Changed 9 years ago by
Status:  needs_work → needs_review 

comment:16 followup: 49 Changed 9 years ago by
Just some general "toplevel" remarks, without really looking at the code yet:
 In
sage/modular/all.py
, theimport *
commands may not be what you want; this will all end up in the global namespace.  It would be nice to add the new modules to the relevant
src/doc/en/reference/*/index.rst
files, probably with*
=modfrm
,modmisc
and/ormodsym
.
comment:17 Changed 9 years ago by
Hi Marc, Jen and I worked on reviewing this last weekend, but I'm getting married on Saturday and haven't had time to upload our progress, etc. I'll try to have some comments and changes next week.
comment:18 Changed 9 years ago by
Status:  needs_review → needs_work 

patchbot:
sage t long src/sage/doctest/sources.py # 1 doctest failed sage t long src/sage/modular/pollack_stevens/distributions.py # 1 doctest failed sage t long src/sage/modular/btquotients/pautomorphicform.py # 2 doctests failed
comment:19 Changed 9 years ago by
Branch:  u/mmasdeu/moved_from_githubfixed_several_bugs → u/chapoton/812 

Commit:  a875b16d34b5e1a3ddf806b19770fa967b104443 → cea37f9e2dedd4de589128c7fa4ec56fb8eaf6a7 
I have made some large scale cleanup of the file btquotient (pyflakes and pep8 compliance). And also taken care of the "doctest continuation" plugin failure.
New commits:
123ab46  Merge branch 'u/mmasdeu/moved_from_githubfixed_several_bugs' of ssh://trac.sagemath.org:22/sage into 812

cea37f9  trac #812 big cleanup of src/sage/modular/btquotients/btquotient.py

comment:20 Changed 9 years ago by
Commit:  cea37f9e2dedd4de589128c7fa4ec56fb8eaf6a7 → 42b0b57f05715fb508300adbede6c58457a239ae 

Branch pushed to git repo; I updated commit sha1. New commits:
42b0b57  trac #812 work on the file pautomorphicform

comment:21 Changed 9 years ago by
Commit:  42b0b57f05715fb508300adbede6c58457a239ae → cecf931929916d51b78854fde6ff99fe190e9645 

Branch pushed to git repo; I updated commit sha1. New commits:
cecf931  trac #812 solving one of the doctest issues

comment:22 Changed 9 years ago by
Commit:  cecf931929916d51b78854fde6ff99fe190e9645 → 8e877fd71ca8dd353f133c64ce882ceec507957d 

Branch pushed to git repo; I updated commit sha1. New commits:
8e877fd  trac #812 more work on pep8 compliance

comment:23 Changed 9 years ago by
Commit:  8e877fd71ca8dd353f133c64ce882ceec507957d → 547130873f2c0e5a0a2035ded64a9fe63c835bbf 

Branch pushed to git repo; I updated commit sha1. New commits:
5471308  trac #812 pyflakes cleanup in pollackstevens directory

comment:24 Changed 9 years ago by
Commit:  547130873f2c0e5a0a2035ded64a9fe63c835bbf → fb13edadda89c4157a901f3c98657b0b5cc0887f 

Branch pushed to git repo; I updated commit sha1. New commits:
fb13eda  trac #812 more pep8 compliance effort

comment:25 Changed 9 years ago by
Commit:  fb13edadda89c4157a901f3c98657b0b5cc0887f → b6b2f79f6ca68ec6fcea94d14f65475ff9e0a12f 

Branch pushed to git repo; I updated commit sha1. New commits:
b6b2f79  trac #812 last (?) step of pep8 cleanup

comment:26 Changed 9 years ago by
Branch:  u/chapoton/812 → u/mmasdeu/812 

Commit:  b6b2f79f6ca68ec6fcea94d14f65475ff9e0a12f → ef137851b7f1f5cbe4f4d5638462fadeca07da8f 
Status:  needs_work → needs_review 
comment:27 Changed 8 years ago by
Status:  needs_review → needs_work 

one doctest is failing, and needs more doc, see patchbot report
comment:28 Changed 8 years ago by
Branch:  u/mmasdeu/812 → u/roed/812 

comment:29 Changed 7 years ago by
Commit:  ef137851b7f1f5cbe4f4d5638462fadeca07da8f → fee5b0ab2ec6a5e692521fdc66ca18631aca37f9 

comment:30 Changed 7 years ago by
Commit:  fee5b0ab2ec6a5e692521fdc66ca18631aca37f9 → cfcf3a9db8c8b0b48fd3bcda341b20507a1fcfc2 

Branch pushed to git repo; I updated commit sha1. New commits:
cfcf3a9  Fix some changed imports due to upgrade to 7.0

comment:32 Changed 7 years ago by
Thanks for asking. It's not yet: there are still doctest failures. I was setting up an SMC project with this code and updated it to 7.0, so I figured I would push the changes here. But there's more work to be done before Needs Review, which I don't have time to do this week.
comment:33 Changed 7 years ago by
It would be nice to get this in at long last. It's been an open Sage ticket since the dawn of time. Working on this at SD44 in Madison must have absorbed hundreds of personhours and thousands of dollars of grant money, and we all flew home thinking the job was 95% done; that was three years ago.
Can you say what would be the absolute minimum amount of work needed for this to get merged? Is it just a case of identifying the doctest failures and fixing them?
comment:34 Changed 7 years ago by
Agreed. I think mostly it's a matter of fixing the doctest problems, and then splitting off further problems into followup tickets. I will try to work on it soon....
comment:35 Changed 7 years ago by
Authors:  → Marc Masdeu, David Roe 

I have added the authors (please check), so that patchbots can run on this ticket once it is put in needs_review.
comment:36 Changed 7 years ago by
I put this into needs_review, so that the patchbots can work on it.
comment:37 Changed 7 years ago by
Status:  needs_work → needs_review 

comment:38 Changed 7 years ago by
Branch:  u/roed/812 → u/mmasdeu/812 

comment:39 Changed 7 years ago by
Cc:  wuthrich added 

Commit:  cfcf3a9db8c8b0b48fd3bcda341b20507a1fcfc2 → ee614e803e57906eba254af17cb548ba9f9b6e74 
New commits:
59c7829  Fixed several bugs. Distributions part is now ready to review.

8fd3ef8  Fixed bug with Tq_eigenvalue and overconvergent lifting.

6c3b88d  Fixed remaining bugs. Only need to change some doctests to reflect new conventions on distribution normalization.

c906011  Fixed failing doctests due to change in dist normalization.

f9d4076  Marked most tests in padic_lseries.py as long time.

ee614e8  Merge branch 'develop' into t/812/812working

comment:40 Changed 7 years ago by
Commit:  ee614e803e57906eba254af17cb548ba9f9b6e74 → fdedb5e340f2910df514de0ab32872070be3544c 

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
e3dd760  Removed fast_dist_act, which was buggy.

cc4f12f  Set normalization in distributions to include zeroth moment by default.

a4cfc3d  Increased output precision in one doctest.

69d22b0  Changed calls from ps_modsym_from_elliptic_curve to e.c. method modular_symbol.

db6283c  Fixed failing doctests.

f4724bf  Fixed remaining doctests for elliptic curves.

8f908aa  Changed use_eclib parameter to implementation one.

5203283  Fixed wrong doctests, rebased code to work again.

6f077dc  Fixed whitespace.

fdedb5e  Marked some doctests as not tested or long time.

comment:41 Changed 7 years ago by
All doctests pass now, finally! I have also checked for whitespace. There are still things that one can work on, but my feeling is that they should be relegated to other tickets depending on this one. Here is a list of these:
 The distributions code is embarrassingly slow. It would be drastically improved by having an implementation that uses Sage's integers (no need to use longs here) instead of Qp and Zp.
 More functions/methods to interact with other implementations of modular symbols could be added. For example, it is straightforward to have a method to construct a PollackStevens modular symbol from a onedimensional modular symbols space. Such a method exists if one starts from an elliptic curve already.
 The padic Lfunction returned by this implementation is quite different to what is returned by the current (not overconvergent) implementation. Fixing this involves deciding where the overconvergent lift is done: one needs to know a target precision for this, whereas the nonoverconvergent implementation only requires the precision parameter when evaluating a particular term of the series.
 The normalization of the padic Lseries is different than the one used in the current implementation. This is noted in the examples, and it can be easy to fix (once this ticket is accepted).
I am sure that there are many other improvements that other people will suggest, but let's first ensure that this ticket doesn't see its 10th birthday.
comment:42 Changed 7 years ago by
See the latest patchbot report for several failing plugins, that needs to be corrected. And doc does not build.
comment:43 Changed 7 years ago by
Commit:  fdedb5e340f2910df514de0ab32872070be3544c → b9b76b00590f55f99414e5070f489309b143a022 

Branch pushed to git repo; I updated commit sha1. New commits:
b9b76b0  Minor fixes to make the patchbot happy. Hopefully docs build now.

comment:44 Changed 7 years ago by
Thanks. But doc still does not build. The problem is probably here:
+  ``sign``  None (default), 0, +1 or 1. If None, choose the default + according to the implementation, which currently is 0 for pollackstevens, + and 1 otherwise.
where the last two lines should be indented by 2.
You can check that the doc build using something like that
sage docbuild reference/plane_curves html
You also used a wrong newstyle doctest continuation, the correct one is ....:
Please correct.
comment:45 Changed 7 years ago by
Commit:  b9b76b00590f55f99414e5070f489309b143a022 → 9e0a5dda4448a1d0353f7280c9731cc5686adf66 

Branch pushed to git repo; I updated commit sha1. New commits:
9e0a5dd  Properly indented two lines.

comment:46 Changed 7 years ago by
Commit:  9e0a5dda4448a1d0353f7280c9731cc5686adf66 → 9e8fed142569eccd54803c10c39c31665b0b561b 

Branch pushed to git repo; I updated commit sha1. New commits:
9e8fed1  Fixed wrong new style .... into ....: .

comment:47 Changed 7 years ago by
<rant>Am I the only one that thinks that this is becoming ridiculous?</rant>
comment:48 Changed 7 years ago by
I'm looking at it now Marc. It is really annoying how many little things come up.
comment:49 Changed 7 years ago by
I think the following points from comment:16 are still valid:
 In
sage/modular/all.py
, theimport *
commands may not be what you want; this will all end up in the global namespace. It would be nice to add the new modules to the relevant
src/doc/en/reference/*/index.rst
files, probably with*
=modfrm
,modmisc
and/ormodsym
.
comment:50 followup: 51 Changed 7 years ago by
Peter,
 The import * commands are not added by this ticket, which only concerns .../pollack_stevens and .../btquotients. In sage/modular/all.py we do import everything in btquotients/all.py (and same for pollack_stevens) but this should be fine, since that file contains only the modules that need to be put in the global namespace.
 I am working on the docs now.
comment:51 followup: 52 Changed 7 years ago by
Hi Marc,
The import * commands are not added by this ticket, which only concerns .../pollack_stevens and .../btquotients. In sage/modular/all.py we do import everything in btquotients/all.py (and same for pollack_stevens) but this should be fine, since that file contains only the modules that need to be put in the global namespace.
It is actually less bad than I thought, but if I understand the import statements correctly, all of the following will end up in the global namespace:
BTQuotient
DoubleCosetReduction
HarmonicCocycleElement
HarmonicCocycles
pAutomorphicFormElement
pAutomorphicForms
PSModularSymbols
Distributions
Symk
ManinRelations
pAdicLseries
I think most mathematicians who are not specialists in automorphic forms couldn't guess what any of these means, let alone the average Sage user...
comment:52 followup: 53 Changed 7 years ago by
Replying to pbruin:
BTQuotient
DoubleCosetReduction
 removedHarmonicCocycleElement
 removedHarmonicCocycles
pAutomorphicFormElement
 removedpAutomorphicForms
PSModularSymbols
Distributions
Symk
ManinRelations
 removedpAdicLseries
 removedI think most mathematicians who are not specialists in automorphic forms couldn't guess what any of these means, let alone the average Sage user...
So the new proposed list would be:
BTQuotient
HarmonicCocycles
pAutomorphicForms
 `PSModularSymbols
Distributions
Symk
Would this be more reasonable? Considering that objects like CrystalOfProjectedLevelZeroLSPaths
is also in the global namespace, I mean. Also, have you seen Magma's global namespace? It doesn't prevent people from using it :).
comment:53 followup: 54 Changed 7 years ago by
Replying to mmasdeu:
So the new proposed list would be:
BTQuotient
HarmonicCocycles
pAutomorphicForms
 `PSModularSymbols
Distributions
Symk
Would this be more reasonable? Considering that objects like
CrystalOfProjectedLevelZeroLSPaths
is also in the global namespace, I mean.
This does sound more reasonable, although Distributions
, HarmonicCocycles
and Symk
should definitely get a more descriptive name that makes it clear that they are padic objects (all of them would make sense in the complex world or in even greater generality).
By the way, I do hope you agree that having something called CrystalOfProjectedLevelZeroLSPaths
in the global namespace is pretty ridiculous and should be deprecated...
Also, have you seen Magma's global namespace? It doesn't prevent people from using it :).
Well, many things are functions in Magma that are methods in Sage, so in principle the global namespace should be cleaner in Sage...
Edit: And maybe pAutomorphicForms
should be called pAdicAutomorphicForms
? Also, I guess it would be less cryptic to expand BT
to BruhatTits
and PS
to PollackStevens
.
comment:54 Changed 7 years ago by
This does sound more reasonable, although
Distributions
,HarmonicCocycles
andSymk
should definitely get a more descriptive name that makes it clear that they are padic objects (all of them would make sense in the complex world or in even greater generality).
I am renaming HarmonicCocycles? to BTHarmonicCocycles, and Distributions to OverconvergentDistributions?. The Symk does not need to be padic, it's pretty general.
By the way, I do hope you agree that having something called
CrystalOfProjectedLevelZeroLSPaths
in the global namespace is pretty ridiculous and should be deprecated...
I do agree, but it's quite low in my list of priorities. I'd rather have Sage create an Eichler order in a quaternion algebra...
Edit: And maybe
pAutomorphicForms
should be calledpAdicAutomorphicForms
? Also, I guess it would be less cryptic to expandBT
toBruhatTits
andPS
toPollackStevens
.
I agree, too.
comment:55 Changed 7 years ago by
Commit:  9e8fed142569eccd54803c10c39c31665b0b561b → 08f9271d7391cbb949dccb5ca45ad5501fadf0c3 

Branch pushed to git repo; I updated commit sha1. New commits:
08f9271  Fixed documentation. Changed imports.

comment:56 Changed 7 years ago by
Commit:  08f9271d7391cbb949dccb5ca45ad5501fadf0c3 → 990f9b804dc653d3b0a2ecf9946dba26584f9481 

Branch pushed to git repo; I updated commit sha1. New commits:
990f9b8  More tiny fixes in documentation.

comment:58 Changed 7 years ago by
OK, thanks! I still haven't got the time for a real review, unfortunately...
comment:60 Changed 6 years ago by
Status:  needs_review → needs_work 

comment:61 Changed 6 years ago by
I started to work on this a bit.
I will try to merge it. But I also found a lot of things that I am not totally happy with, before that ticket goes in. See below. There is a real danger that this patchbomb ticket never makes it into sage at this rate. I would be really sorry about this. Here are the possibilities:
 One option would be to split it up: Put the basic code (i.e. the new files) in then in further tickets starts incorporating it into sage step by step.
 Another option is to accept it in an incomplete version as soon as it is rebased and doctest are corrected and then open new tickets. But that means that certain functions will change and deprecation will be all over the place.
 We try to fix all issues here slowly and hope this converges in finite time.
Right, option 1 would have been the correct decision at the start but I fear it is too late for this. Marc probably favours 2 and I would prefer 3, I must admit, although I am not sure I can put in the effort needed.
comment:62 Changed 6 years ago by
(I am having issues posting. So I will do it in small portions, sorry)
Now here to a list of issues that I spotted this morning.
 I don't think it is a good idea that
m = E.modular_symbol(implementation="pollackstevens")
gives a completely different type of thing back. It is not even callable. Is it even desirable to have them in the same function. I would have leftmodular_symbols
as before and added aoverconvergent_modular_symbols
as a separate method. (Personally, I would revert this function to before and think about it in a next ticket.)
comment:63 Changed 6 years ago by
 The normalisation of the padic Lfunction is wrong. One has to divide by the number of real connected components of the elliptic curve. This matters for p=2, otherwise it is a choice of normalisation. But it is better to be consistent with sage.
 If we leave
E.modular_symbol()
as modified here, then it should take eclib as default (as stated in the new docstring). Without having resolved #10256,ps_modsym_from_elliptic_curve
should take eclib for positive and sage for negative. Or is sage for both a better choice? Certainly much slower. (Again I opt, for keeping as before and use slow sage for overconvergent ms by now)
comment:64 Changed 6 years ago by
 The arguments
n
andprec
in series of the overconvergent padic Lfunctions are not clear to me. They are probably the wrong way around. The new precision should have a default value, also it should have another name, not to confuse it with the precision in T.
 Many functions have incomplete documentations. E.g. no description of what the input arguments mean and what the output means.
If there is anyone else working on this, please let me know. I might not update my changes too often, but I will try to work on these issues over the next two weeks.
comment:65 Changed 6 years ago by
Branch:  u/mmasdeu/812 → u/mmasdeu/812fixchris 

Commit:  990f9b804dc653d3b0a2ecf9946dba26584f9481 → 3e3c32c70b93204671dc5dc792854bb7bf15a360 
Description:  modified (diff) 
Reviewers:  → Chris Wuthrich 
Last 10 new commits:
a9b9598  trac 812: divide by c_oo

a60802d  trac 812:renaming, working on doctests

25b9383  trac 812: change names, adjust docstrings

779430c  trac 812: improve docstrings

0c1eed9  trac 812: more docstrings

bd26252  trac 812: docstrings again

8b335d9  trac 812: long doctests

58b60ca  trac 812: revert overconvergent to pollack stevens

368b702  Fixed some of the problems encountered by Chris Wuthrich.

3e3c32c  Removed one comment about different normalizations  they are the same now.

comment:66 Changed 6 years ago by
Branch:  u/mmasdeu/812fixchris → u/wuthrich/ticket/812 

Commit:  3e3c32c70b93204671dc5dc792854bb7bf15a360 → c0fa87dd20ce75bc7fc4be93576dd930ce77c77b 
Last 10 new commits:
a60802d  trac 812:renaming, working on doctests

25b9383  trac 812: change names, adjust docstrings

779430c  trac 812: improve docstrings

0c1eed9  trac 812: more docstrings

bd26252  trac 812: docstrings again

8b335d9  trac 812: long doctests

58b60ca  trac 812: revert overconvergent to pollack stevens

368b702  Fixed some of the problems encountered by Chris Wuthrich.

3e3c32c  Removed one comment about different normalizations  they are the same now.

c0fa87d  trac 812: minor last changes

comment:67 Changed 6 years ago by
Thanks Marc, for fixing the last problems and improving the docstrings. A spotted a few last things. Now there is one very last thing that should be changed before this goes in: Please change the title line of pollack_stevens/modsym.py. I will be running the tests.
I opend a few follow up tickets:
comment:68 Changed 6 years ago by
Milestone:  sagefeature → sage7.3 

comment:69 Changed 6 years ago by
please take care of using python2/3 compatible print. See patchbot report (plugin oldstyle_print)
and
comment:70 Changed 6 years ago by
Commit:  c0fa87dd20ce75bc7fc4be93576dd930ce77c77b → 4ba901c4d2e0d0d9d5abea38b82c662f333e2891 

It only screamed about commented lines. I changed them too.
(I am not sure the bot should tell us it failed when print is wrongly used on comment lines and I am very certain it should say nothing about the "print" apprearing in the middle of a docstring as in "Return the print representation" in _repr_
)
New commits:
4ba901c  trac 812: print in python3 also in commented lines

comment:71 Changed 6 years ago by
Indeed, the plugin is not very smart, and should not care about commented lines. Sorry for the noise.
comment:72 Changed 6 years ago by
It all compiles fine now and tests all pass. So if that one title line is changed this can be set to positive review.
comment:73 Changed 6 years ago by
Branch:  u/wuthrich/ticket/812 → u/mmasdeu/812fixtitle 

Commit:  4ba901c4d2e0d0d9d5abea38b82c662f333e2891 → 2ede1469bd9e2e79cb0210b28c1078f677480f98 
Status:  needs_work → needs_review 
Done. I have also changed the title in space.py, since the modular symbols in it are not necessarily overconvergent.
New commits:
2ede146  Added title to modsym.py and changed title in space.py

comment:74 Changed 6 years ago by
Status:  needs_review → positive_review 

Thanks Marc.
This 9 year old ticket is now done ! Yeah.
comment:75 Changed 6 years ago by
Status:  positive_review → needs_work 

[sagelib7.3.beta4] [2/7] Cythonizing sage/modular/pollack_stevens/dist.pyx [sagelib7.3.beta4] [3/7] Cythonizing sage/numerical/backends/cplex_backend.pyx [sagelib7.3.beta4] [sagelib7.3.beta4] Error compiling Cython file: [sagelib7.3.beta4]  [sagelib7.3.beta4] ... [sagelib7.3.beta4] if negate: [sagelib7.3.beta4] rmoments = rmoments [sagelib7.3.beta4] ans._moments = smoments + rmoments [sagelib7.3.beta4] return ans [sagelib7.3.beta4] [sagelib7.3.beta4] cpdef ModuleElement _add_(self, ModuleElement _right): [sagelib7.3.beta4] ^ [sagelib7.3.beta4]  [sagelib7.3.beta4] [sagelib7.3.beta4] sage/modular/pollack_stevens/dist.pyx:957:10: Signature not compatible with previous declaration
comment:76 Changed 6 years ago by
Branch:  u/mmasdeu/812fixtitle → u/mmasdeu/812pollackstevens 

Commit:  2ede1469bd9e2e79cb0210b28c1078f677480f98 → 3a54bd99518ffe2b6f2fc2654208d26937702f05 
Status:  needs_work → needs_review 
I don't know how to reproduce the above error in my machine. I removed a superfluous import (in dist.pyx and dist.pxd), which I think is unrelated but was giving out warnings while cythonizing. Also, to make the patchbot happy I changed "print" into "string" in some of the docstrings.
New commits:
afeb1a1  Trac #812: Removed unneeded imports.

3a54bd9  Trac #812: Changed "print" to "string" in some of the docstrings to avoid bug in patchbot.

comment:77 Changed 6 years ago by
Hello. You should not care about stupid patchbot remarks. Having the patchbot complete approval is not a requirement, as long as you understand why.
comment:78 followup: 79 Changed 6 years ago by
I'm just worried about Volker's remarks, not the patchbot's. Can someone see how to fix the issue (or at least how to reproduce it?). I have looked at the signature in sage/structure/element.pxd and it looks the same as the one used in dist.pyx...
comment:79 Changed 6 years ago by
Replying to mmasdeu:
I'm just worried about Volker's remarks, not the patchbot's. Can someone see how to fix the issue (or at least how to reproduce it?). I have looked at the signature in sage/structure/element.pxd and it looks the same as the one used in dist.pyx...
It must be because of #20740, which is apparently merged in Volker's branch, but is not in the latest beta.
comment:80 Changed 6 years ago by
Status:  needs_review → needs_work 

I think that is right. Even with the updated branch here, when merged into the current develop, there are Error compiling Cython file: dist.pyx.
comment:81 Changed 6 years ago by
Commit:  3a54bd99518ffe2b6f2fc2654208d26937702f05 → 4374c5b2ef7f680e9929e2d0b87270d9376475f7 

comment:85 Changed 6 years ago by
Status:  positive_review → needs_work 

Failures on 32bit:
sage t long src/sage/modular/btquotients/btquotient.py ********************************************************************** File "src/sage/modular/btquotients/btquotient.py", line 1502, in sage.modular.btquotients.btquotient.BruhatTitsQuotient._cache_key Failed example: X._cache_key() Expected: 1375458358400022881 Got: 406423199 ********************************************************************** File "src/sage/modular/btquotients/btquotient.py", line 2478, in sage.modular.btquotients.btquotient.BruhatTitsQuotient.embed_quaternion Failed example: l[3] Expected: [1] [ 0] [ 1] [ 1] Got: [1] [ 1] [ 0] [ 1] ********************************************************************** File "src/sage/modular/btquotients/btquotient.py", line 2483, in sage.modular.btquotients.btquotient.BruhatTitsQuotient.embed_quaternion Failed example: X.embed_quaternion(l[3]) Expected: [ O(7) 3 + O(7)] [2 + O(7) 6 + O(7)] Got: [4 + O(7) O(7)] [1 + O(7) 2 + O(7)] ********************************************************************** File "src/sage/modular/btquotients/btquotient.py", line 2487, in sage.modular.btquotients.btquotient.BruhatTitsQuotient.embed_quaternion Failed example: X.embed_quaternion(l[3]) Expected: [ 7 + 3*7^2 + 7^3 + 4*7^4 + O(7^6) 3 + 7 + 3*7^2 + 7^3 + 4*7^4 + O(7^6)] [ 2 + 7 + 3*7^2 + 7^3 + 4*7^4 + O(7^6) 6 + 5*7 + 3*7^2 + 5*7^3 + 2*7^4 + 6*7^5 + O(7^6)] Got: [4 + 5*7 + 3*7^2 + 5*7^3 + 2*7^4 + 6*7^5 + O(7^6) 7 + 3*7^2 + 7^3 + 4*7^4 + O(7^6)] [ 1 + 7 + 3*7^2 + 7^3 + 4*7^4 + O(7^6) 2 + 7 + 3*7^2 + 7^3 + 4*7^4 + O(7^6)] ********************************************************************** 2 items had failures: 1 of 3 in sage.modular.btquotients.btquotient.BruhatTitsQuotient._cache_key 3 of 8 in sage.modular.btquotients.btquotient.BruhatTitsQuotient.embed_quaternion [375 tests, 4 failures, 8.18 s]
comment:86 Changed 6 years ago by
Commit:  4374c5b2ef7f680e9929e2d0b87270d9376475f7 → d5fed29f178c22211c76f0b5875b4d1329573c80 

Branch pushed to git repo; I updated commit sha1. New commits:
d5fed29  Marked some doctests with random output. Fixed problem with magma maximal order.

comment:87 Changed 6 years ago by
That is fine by me. Is the first output really random ? Otherwise you can use # 32bit
and # 64bit
in the doctest.
In any case, I have no means of checking as I don't have access to magma or 32bit. If someone listening has, they should run the test for us, otherwise we have to bother the release manager again with a "positive review"....
comment:88 Changed 6 years ago by
I fixed the problem with the doctests, plus another bug that arised when using Magma. Tests in btquotients and in pollackstevens pass in my machine.
comment:89 Changed 6 years ago by
The __cache_key
is not random, but I have put it as it were since it is not of much use. The test done after is more relevant. The embedding functions depend on a call to pMatrixRing
in Magma, so it would be random, or at least fairly unpredictable.
comment:90 Changed 6 years ago by
Status:  needs_work → needs_review 

comment:91 Changed 6 years ago by
You can use this:
sage: hash(SR(19.23)) 1458111714 # 32bit 2836855582 # 64bit
comment:92 Changed 6 years ago by
Commit:  d5fed29f178c22211c76f0b5875b4d1329573c80 → d94b19ec7f8dd6ef619bb2a47c5233a9718aa6cf 

Branch pushed to git repo; I updated commit sha1. New commits:
d94b19e  Changed doctest to allow 32/64 bit output.

comment:93 Changed 6 years ago by
I have followed Volker's suggestion. I leave it as needs_review...
comment:94 Changed 6 years ago by
Status:  needs_review → positive_review 

To me all tests still pass and I can't check 32bit or optional magma. So I set this to positive review, fearing of course that a bot or vbraun will find yet another issue.
The patchbot complains about something, but I don't understand it.
comment:95 Changed 6 years ago by
Commit:  d94b19ec7f8dd6ef619bb2a47c5233a9718aa6cf → d11150c160981e911468854be4a493d44c56b6d9 

Status:  positive_review → needs_review 
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
d11150c  Moved an auxiliary function where it was used.

comment:96 Changed 6 years ago by
I realized that there was one doctest missing in btquotient.py
. This is fixed now. I'm keeping it as "needs review" and hope that the patchbot is happier now.
comment:97 Changed 6 years ago by
Well, you deleted lots of commented code and moved an undocumented function to a place where coverage won't complain about it. I guess this is not the optimal solutions, but one can deal with this in #20863.
Coverage also complains about another missing docstring:
SCORE src/sage/schemes/elliptic_curves/padics.py: 92.3% (12 of 13) Missing doctests: * line 72: def _normalize_padic_lseries(self, p, normalize, use_eclib, implementation, precision)
comment:98 Changed 6 years ago by
Commit:  d11150c160981e911468854be4a493d44c56b6d9 → 31a3e87ceeb7ba39b884ef22fc7adffb18c841fc 

Branch pushed to git repo; I updated commit sha1. New commits:
31a3e87  Added missing doctest.

comment:99 Changed 6 years ago by
comment:101 Changed 6 years ago by
Status:  positive_review → needs_work 

Building the pdf docs fails
[docpdf] LaTeX Warning: Hyper reference `sage/modular/pollack_stevens/modsym:sage.modula [docpdf] r.pollack_stevens.modsym.PSModularSymbolElement' on page 162 undefined on input [docpdf] line 13297. [docpdf] [docpdf] [162] [163] [164] [165] [166] [docpdf] Underfull \hbox (badness 10000) in paragraph at lines 1362813628 [docpdf] \T1/ptm/m/it/10 character=None\T1/ptm/m/n/10 , [docpdf] [docpdf] Underfull \hbox (badness 10000) in paragraph at lines 1362813628 [docpdf] \T1/ptm/m/it/10 use_magma=False\T1/ptm/m/n/10 , [docpdf] [docpdf] Overfull \hbox (152.67451pt too wide) in paragraph at lines 1362913630 [docpdf] \T1/ptm/m/n/10 Bases: \T1/pcr/m/n/10 sage.structure.sage_object.SageObject\T1/p [docpdf] tm/m/n/10 , \T1/pcr/m/n/10 sage.structure.unique_representation.UniqueRepresent [docpdf] ation [docpdf] [167] [168] [169] [170] [171] [172] [173] [174] [175] [176] [177] [178] [docpdf] Overfull \hbox (152.67451pt too wide) in paragraph at lines 1489014891 [docpdf] \T1/ptm/m/n/10 Bases: \T1/pcr/m/n/10 sage.structure.sage_object.SageObject\T1/p [docpdf] tm/m/n/10 , \T1/pcr/m/n/10 sage.structure.unique_representation.UniqueRepresent [docpdf] ation [docpdf] [179] [180] [181] [docpdf] ! Undefined control sequence. [docpdf] l.15130 modulo \(p\) is not in \(\FF [docpdf] _p\): [docpdf] ? [docpdf] ! Emergency stop. [docpdf] l.15130 modulo \(p\) is not in \(\FF [docpdf] _p\): [docpdf] ! ==> Fatal error occurred, no output PDF file produced! [docpdf] Transcript written on modsym.log. [docpdf] Makefile:68: recipe for target 'modsym.pdf' failed
comment:102 Changed 6 years ago by
Branch:  u/mmasdeu/812pollackstevens → u/wuthrich/ticket/812 

Commit:  31a3e87ceeb7ba39b884ef22fc7adffb18c841fc → 44c321f58f42ec0b11fbe8674545687b3562e73a 
pdf documentation now build for me.
Last 10 new commits:
afeb1a1  Trac #812: Removed unneeded imports.

3a54bd9  Trac #812: Changed "print" to "string" in some of the docstrings to avoid bug in patchbot.

32fa647  Trac 812: changed signature of ModuleElement.

cef9a4a  Merge branch 'develop' into 812chris2

4374c5b  Trac #812: finished adapting the signatures.

d5fed29  Marked some doctests with random output. Fixed problem with magma maximal order.

d94b19e  Changed doctest to allow 32/64 bit output.

d11150c  Moved an auxiliary function where it was used.

31a3e87  Added missing doctest.

44c321f  trac 812: docstring changes for pdf

comment:103 Changed 6 years ago by
Status:  needs_work → positive_review 

comment:104 Changed 6 years ago by
Branch:  u/wuthrich/ticket/812 → 44c321f58f42ec0b11fbe8674545687b3562e73a 

Resolution:  → fixed 
Status:  positive_review → closed 
Craig,
I know I must be bothering you by now, but what is the status here?
Cheers,
Michael