Opened 11 years ago

Closed 3 years ago

#812 closed enhancement (fixed)

add Pollack/Stevens overconvergent modular symbols code

Reported by: craigcitro Owned by: mmasdeu
Priority: major Milestone: sage-7.3
Component: modular forms Keywords: p-adic L-functions
Cc: davidloeffler, roed, craigcitro, wuthrich Merged in:
Authors: Marc Masdeu, David Roe Reviewers: Chris Wuthrich
Report Upstream: N/A Work issues:
Branch: 44c321f (Commits) Commit: 44c321f58f42ec0b11fbe8674545687b3562e73a
Dependencies: Stopgaps:

Description (last modified by wuthrich)

I'm just starting to work on implementing Pollack & Stevens' methods for using overconvergent modular symbols for p-adic L-functions, Stark-Heegner points, etc.

Change History (104)

comment:1 Changed 11 years ago by craigcitro

  • Status changed from new to assigned

comment:2 Changed 11 years ago by craigcitro

  • Cc craigcitro@… added

comment:3 Changed 11 years ago by mabshoff

Craig,

I know I must be bothering you by now, but what is the status here?

Cheers,

Michael

comment:4 Changed 10 years ago by was

Could somebody post a link to the Magma code here?

comment:5 Changed 9 years ago by GeorgSWeber

Sure, it's part of the "shp package" at:

http://www.math.mcgill.ca/darmon/programs/shp/shp.html

comment:6 Changed 7 years ago by was

  • Report Upstream set to N/A

From Jennifer Balakrishnan:

Rob Pollack just ported over some of his p-adic L-series via overconvergent modular symbols code to Sage, which could be very useful for our p-adic 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 p-adic L-series 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 L-value!! -- 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 6 years ago by was

  • Description modified (diff)

comment:8 Changed 5 years ago by mmasdeu

  • Branch set to u/mmasdeu/moved_from_github-fixed_several_bugs
  • Commit set to 9acf3ac74066be652fe7d28801c577d48a4f387d
  • Owner changed from craigcitro to mmasdeu

New commits:

1b85377Initial commit. Cloned from experimental branch of roed314's github.
1656711Made all tests to pass.
72923abIncreased coverage.
caaa672Fixed precision of p_stabilize. Two more tests pass.
c0331a5Fixed one test.
9fd7d69All files pass the doctests except padic_lseries.py
4bbe597All doctests passed, have 100% coverage.
c5aa41dRemoved ocmodule.py
ce76f3cRemoved references to old OCVn.
9acf3acFixed one doctest.

comment:9 Changed 5 years ago by mmasdeu

  • Cc davidloeffler roed craigcitro added; craigcitro@… removed
  • Status changed from new to needs_review

comment:10 Changed 5 years ago by mmasdeu

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 5 years ago by chapoton

  • Status changed from needs_review to needs_work

There are some failing doctests, see patchbot's report

comment:12 Changed 5 years ago by git

  • Commit changed from 9acf3ac74066be652fe7d28801c577d48a4f387d to a875b16d34b5e1a3ddf806b19770fa967b104443

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

a875b16Fixed a doctest which failed when run without magma and long time flag enabled.

comment:13 Changed 5 years ago by mmasdeu

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 5 years ago by was

I just (very) quickly skimmed the patches -- this is some beautiful code!

comment:15 Changed 5 years ago by mmasdeu

  • Status changed from needs_work to needs_review

comment:16 follow-up: Changed 5 years ago by pbruin

Just some general "top-level" remarks, without really looking at the code yet:

  • In sage/modular/all.py, the import * 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/or modsym.

comment:17 Changed 5 years ago by roed

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 5 years ago by rws

  • Status changed from needs_review to 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 5 years ago by chapoton

  • Branch changed from u/mmasdeu/moved_from_github-fixed_several_bugs to u/chapoton/812
  • Commit changed from a875b16d34b5e1a3ddf806b19770fa967b104443 to 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:

123ab46Merge branch 'u/mmasdeu/moved_from_github-fixed_several_bugs' of ssh://trac.sagemath.org:22/sage into 812
cea37f9trac #812 big cleanup of src/sage/modular/btquotients/btquotient.py

comment:20 Changed 5 years ago by git

  • Commit changed from cea37f9e2dedd4de589128c7fa4ec56fb8eaf6a7 to 42b0b57f05715fb508300adbede6c58457a239ae

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

42b0b57trac #812 work on the file pautomorphicform

comment:21 Changed 5 years ago by git

  • Commit changed from 42b0b57f05715fb508300adbede6c58457a239ae to cecf931929916d51b78854fde6ff99fe190e9645

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

cecf931trac #812 solving one of the doctest issues

comment:22 Changed 5 years ago by git

  • Commit changed from cecf931929916d51b78854fde6ff99fe190e9645 to 8e877fd71ca8dd353f133c64ce882ceec507957d

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

8e877fdtrac #812 more work on pep8 compliance

comment:23 Changed 5 years ago by git

  • Commit changed from 8e877fd71ca8dd353f133c64ce882ceec507957d to 547130873f2c0e5a0a2035ded64a9fe63c835bbf

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

5471308trac #812 pyflakes clean-up in pollack-stevens directory

comment:24 Changed 5 years ago by git

  • Commit changed from 547130873f2c0e5a0a2035ded64a9fe63c835bbf to fb13edadda89c4157a901f3c98657b0b5cc0887f

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

fb13edatrac #812 more pep8 compliance effort

comment:25 Changed 5 years ago by git

  • Commit changed from fb13edadda89c4157a901f3c98657b0b5cc0887f to b6b2f79f6ca68ec6fcea94d14f65475ff9e0a12f

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

b6b2f79trac #812 last (?) step of pep8 cleanup

comment:26 Changed 5 years ago by mmasdeu

  • Branch changed from u/chapoton/812 to u/mmasdeu/812
  • Commit changed from b6b2f79f6ca68ec6fcea94d14f65475ff9e0a12f to ef137851b7f1f5cbe4f4d5638462fadeca07da8f
  • Status changed from needs_work to needs_review

New commits:

090a4e3Merge branch 'develop' into ticket/812
ef13785Fixes on generators of BTQuotient, found by Peter Graef.

comment:27 Changed 5 years ago by chapoton

  • Status changed from needs_review to needs_work

one doctest is failing, and needs more doc, see patchbot report

comment:28 Changed 4 years ago by roed

  • Branch changed from u/mmasdeu/812 to u/roed/812

comment:29 Changed 3 years ago by git

  • Commit changed from ef137851b7f1f5cbe4f4d5638462fadeca07da8f to fee5b0ab2ec6a5e692521fdc66ca18631aca37f9

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

218ae0bMerge sage-7.0 into t/812/812
fee5b0aFix module_list.py for update to sage-7.0

comment:30 Changed 3 years ago by git

  • Commit changed from fee5b0ab2ec6a5e692521fdc66ca18631aca37f9 to cfcf3a9db8c8b0b48fd3bcda341b20507a1fcfc2

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

cfcf3a9Fix some changed imports due to upgrade to 7.0

comment:31 Changed 3 years ago by chapoton

is this needs review now ?

comment:32 Changed 3 years ago by roed

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 3 years ago by davidloeffler

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 person-hours 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 3 years ago by roed

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 3 years ago by chapoton

  • Authors set to 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 3 years ago by chapoton

I put this into needs_review, so that the patchbots can work on it.

comment:37 Changed 3 years ago by chapoton

  • Status changed from needs_work to needs_review

comment:38 Changed 3 years ago by mmasdeu

  • Branch changed from u/roed/812 to u/mmasdeu/812

comment:39 Changed 3 years ago by wuthrich

  • Cc wuthrich added
  • Commit changed from cfcf3a9db8c8b0b48fd3bcda341b20507a1fcfc2 to ee614e803e57906eba254af17cb548ba9f9b6e74

New commits:

59c7829Fixed several bugs. Distributions part is now ready to review.
8fd3ef8Fixed bug with Tq_eigenvalue and overconvergent lifting.
6c3b88dFixed remaining bugs. Only need to change some doctests to reflect new conventions on distribution normalization.
c906011Fixed failing doctests due to change in dist normalization.
f9d4076Marked most tests in padic_lseries.py as long time.
ee614e8Merge branch 'develop' into t/812/812-working

comment:40 Changed 3 years ago by git

  • Commit changed from ee614e803e57906eba254af17cb548ba9f9b6e74 to fdedb5e340f2910df514de0ab32872070be3544c

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

e3dd760Removed fast_dist_act, which was buggy.
cc4f12fSet normalization in distributions to include zeroth moment by default.
a4cfc3dIncreased output precision in one doctest.
69d22b0Changed calls from ps_modsym_from_elliptic_curve to e.c. method modular_symbol.
db6283cFixed failing doctests.
f4724bfFixed remaining doctests for elliptic curves.
8f908aaChanged use_eclib parameter to implementation one.
5203283Fixed wrong doctests, rebased code to work again.
6f077dcFixed whitespace.
fdedb5eMarked some doctests as not tested or long time.

comment:41 Changed 3 years ago by mmasdeu

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:

  1. 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.
  1. 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 Pollack-Stevens modular symbol from a one-dimensional modular symbols space. Such a method exists if one starts from an elliptic curve already.
  1. The p-adic L-function 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 non-overconvergent implementation only requires the precision parameter when evaluating a particular term of the series.
  1. The normalization of the p-adic L-series 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 3 years ago by chapoton

See the latest patchbot report for several failing plugins, that needs to be corrected. And doc does not build.

comment:43 Changed 3 years ago by git

  • Commit changed from fdedb5e340f2910df514de0ab32872070be3544c to b9b76b00590f55f99414e5070f489309b143a022

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

b9b76b0Minor fixes to make the patchbot happy. Hopefully docs build now.

comment:44 Changed 3 years ago by chapoton

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 pollack-stevens,
+        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.

Last edited 3 years ago by chapoton (previous) (diff)

comment:45 Changed 3 years ago by git

  • Commit changed from b9b76b00590f55f99414e5070f489309b143a022 to 9e0a5dda4448a1d0353f7280c9731cc5686adf66

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

9e0a5ddProperly indented two lines.

comment:46 Changed 3 years ago by git

  • Commit changed from 9e0a5dda4448a1d0353f7280c9731cc5686adf66 to 9e8fed142569eccd54803c10c39c31665b0b561b

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

9e8fed1Fixed wrong new style .... into ....: .

comment:47 Changed 3 years ago by mmasdeu

<rant>Am I the only one that thinks that this is becoming ridiculous?</rant>

comment:48 Changed 3 years ago by roed

I'm looking at it now Marc. It is really annoying how many little things come up.

comment:49 in reply to: ↑ 16 Changed 3 years ago by pbruin

I think the following points from comment:16 are still valid:

  • In sage/modular/all.py, the import * 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/or modsym.

comment:50 follow-up: Changed 3 years ago by mmasdeu

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 in reply to: ↑ 50 ; follow-up: Changed 3 years ago by pbruin

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 in reply to: ↑ 51 ; follow-up: Changed 3 years ago by mmasdeu

Replying to pbruin:

  • BTQuotient
  • DoubleCosetReduction - removed
  • HarmonicCocycleElement - removed
  • HarmonicCocycles
  • pAutomorphicFormElement - removed
  • pAutomorphicForms
  • PSModularSymbols
  • Distributions
  • Symk
  • ManinRelations - removed
  • pAdicLseries - removed

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...

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 in reply to: ↑ 52 ; follow-up: Changed 3 years ago by pbruin

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 p-adic 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.

Last edited 3 years ago by pbruin (previous) (diff)

comment:54 in reply to: ↑ 53 Changed 3 years ago by mmasdeu

This does sound more reasonable, although Distributions, HarmonicCocycles and Symk should definitely get a more descriptive name that makes it clear that they are p-adic 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 p-adic, 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 called pAdicAutomorphicForms? Also, I guess it would be less cryptic to expand BT to BruhatTits and PS to PollackStevens.

I agree, too.

comment:55 Changed 3 years ago by git

  • Commit changed from 9e8fed142569eccd54803c10c39c31665b0b561b to 08f9271d7391cbb949dccb5ca45ad5501fadf0c3

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

08f9271Fixed documentation. Changed imports.

comment:56 Changed 3 years ago by git

  • Commit changed from 08f9271d7391cbb949dccb5ca45ad5501fadf0c3 to 990f9b804dc653d3b0a2ecf9946dba26584f9481

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

990f9b8More tiny fixes in documentation.

comment:57 Changed 3 years ago by mmasdeu

I think it looks good now.

comment:58 Changed 3 years ago by pbruin

OK, thanks! I still haven't got the time for a real review, unfortunately...

comment:59 Changed 3 years ago by chapoton

needs rebase on latest develop

comment:60 Changed 3 years ago by rws

  • Status changed from needs_review to needs_work

comment:61 Changed 3 years ago by wuthrich

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 patch-bomb ticket never makes it into sage at this rate. I would be really sorry about this. Here are the possibilities:

  1. 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.
  1. Another option is to accept it in an incomplete version as soon as it is rebased and doc-test are corrected and then open new tickets. But that means that certain functions will change and deprecation will be all over the place.
  1. 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 3 years ago by wuthrich

(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="pollack-stevens") 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 left modular_symbols as before and added a overconvergent_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 3 years ago by wuthrich

  • The normalisation of the p-adic L-function 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 3 years ago by wuthrich

  • The arguments n and prec in series of the overconvergent p-adic L-functions 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 3 years ago by wuthrich

  • Branch changed from u/mmasdeu/812 to u/mmasdeu/812-fixchris
  • Commit changed from 990f9b804dc653d3b0a2ecf9946dba26584f9481 to 3e3c32c70b93204671dc5dc792854bb7bf15a360
  • Description modified (diff)
  • Reviewers set to Chris Wuthrich

Last 10 new commits:

a9b9598trac 812: divide by c_oo
a60802dtrac 812:renaming, working on doctests
25b9383trac 812: change names, adjust docstrings
779430ctrac 812: improve docstrings
0c1eed9trac 812: more docstrings
bd26252trac 812: docstrings again
8b335d9trac 812: long doctests
58b60catrac 812: revert overconvergent to pollack stevens
368b702Fixed some of the problems encountered by Chris Wuthrich.
3e3c32cRemoved one comment about different normalizations - they are the same now.

comment:66 Changed 3 years ago by wuthrich

  • Branch changed from u/mmasdeu/812-fixchris to u/wuthrich/ticket/812
  • Commit changed from 3e3c32c70b93204671dc5dc792854bb7bf15a360 to c0fa87dd20ce75bc7fc4be93576dd930ce77c77b

Last 10 new commits:

a60802dtrac 812:renaming, working on doctests
25b9383trac 812: change names, adjust docstrings
779430ctrac 812: improve docstrings
0c1eed9trac 812: more docstrings
bd26252trac 812: docstrings again
8b335d9trac 812: long doctests
58b60catrac 812: revert overconvergent to pollack stevens
368b702Fixed some of the problems encountered by Chris Wuthrich.
3e3c32cRemoved one comment about different normalizations - they are the same now.
c0fa87dtrac 812: minor last changes

comment:67 Changed 3 years ago by wuthrich

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:

  • #20863 : Improve documentation. There are some # mm TODO left.
  • #20864 : The caching of modular_symbols should be modernised and the keyword use_eclib should be dropped
  • #20865 : General ticket to ask for improving the code so that the p-adic L-functions are as fast as we could hope for.

comment:68 Changed 3 years ago by chapoton

  • Milestone changed from sage-feature to sage-7.3

comment:69 Changed 3 years ago by chapoton

please take care of using python2/3 compatible print. See patchbot report (plugin oldstyle_print)

and

https://wiki.sagemath.org/PrintFunction

comment:70 Changed 3 years ago by wuthrich

  • Commit changed from c0fa87dd20ce75bc7fc4be93576dd930ce77c77b to 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:

4ba901ctrac 812: print in python3 also in commented lines

comment:71 Changed 3 years ago by chapoton

Indeed, the plugin is not very smart, and should not care about commented lines. Sorry for the noise.

comment:72 Changed 3 years ago by wuthrich

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 3 years ago by mmasdeu

  • Branch changed from u/wuthrich/ticket/812 to u/mmasdeu/812-fixtitle
  • Commit changed from 4ba901c4d2e0d0d9d5abea38b82c662f333e2891 to 2ede1469bd9e2e79cb0210b28c1078f677480f98
  • Status changed from needs_work to needs_review

Done. I have also changed the title in space.py, since the modular symbols in it are not necessarily overconvergent.


New commits:

2ede146Added title to modsym.py and changed title in space.py

comment:74 Changed 3 years ago by wuthrich

  • Status changed from needs_review to positive_review

Thanks Marc.

This 9 year old ticket is now done ! Yeah.

comment:75 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work
[sagelib-7.3.beta4] [2/7] Cythonizing sage/modular/pollack_stevens/dist.pyx
[sagelib-7.3.beta4] [3/7] Cythonizing sage/numerical/backends/cplex_backend.pyx
[sagelib-7.3.beta4] 
[sagelib-7.3.beta4] Error compiling Cython file:
[sagelib-7.3.beta4] ------------------------------------------------------------
[sagelib-7.3.beta4] ...
[sagelib-7.3.beta4]         if negate:
[sagelib-7.3.beta4]             rmoments = -rmoments
[sagelib-7.3.beta4]         ans._moments = smoments + rmoments
[sagelib-7.3.beta4]         return ans
[sagelib-7.3.beta4] 
[sagelib-7.3.beta4]     cpdef ModuleElement _add_(self, ModuleElement _right):
[sagelib-7.3.beta4]          ^
[sagelib-7.3.beta4] ------------------------------------------------------------
[sagelib-7.3.beta4] 
[sagelib-7.3.beta4] sage/modular/pollack_stevens/dist.pyx:957:10: Signature not compatible with previous declaration

comment:76 Changed 3 years ago by mmasdeu

  • Branch changed from u/mmasdeu/812-fixtitle to u/mmasdeu/812-pollackstevens
  • Commit changed from 2ede1469bd9e2e79cb0210b28c1078f677480f98 to 3a54bd99518ffe2b6f2fc2654208d26937702f05
  • Status changed from needs_work to 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:

afeb1a1Trac #812: Removed unneeded imports.
3a54bd9Trac #812: Changed "print" to "string" in some of the docstrings to avoid bug in patchbot.

comment:77 Changed 3 years ago by chapoton

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 follow-up: Changed 3 years ago by 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...

comment:79 in reply to: ↑ 78 Changed 3 years ago by pbruin

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 3 years ago by wuthrich

  • Status changed from needs_review to 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 3 years ago by git

  • Commit changed from 3a54bd99518ffe2b6f2fc2654208d26937702f05 to 4374c5b2ef7f680e9929e2d0b87270d9376475f7

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

32fa647Trac 812: changed signature of ModuleElement.
cef9a4aMerge branch 'develop' into 812-chris2
4374c5bTrac #812: finished adapting the signatures.

comment:82 Changed 3 years ago by mmasdeu

  • Status changed from needs_work to needs_review

I believe that it works now.

comment:83 Changed 3 years ago by wuthrich

It compiles fine for me. Running the tests now.

comment:84 Changed 3 years ago by wuthrich

  • Status changed from needs_review to positive_review

they passed

comment:85 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

Failures on 32-bit:

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 3 years ago by git

  • Commit changed from 4374c5b2ef7f680e9929e2d0b87270d9376475f7 to d5fed29f178c22211c76f0b5875b4d1329573c80

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

d5fed29Marked some doctests with random output. Fixed problem with magma maximal order.

comment:87 Changed 3 years ago by wuthrich

That is fine by me. Is the first output really random ? Otherwise you can use # 32-bit and # 64-bit in the doctest.

In any case, I have no means of checking as I don't have access to magma or 32-bit. 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 3 years ago by mmasdeu

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 3 years ago by mmasdeu

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 3 years ago by mmasdeu

  • Status changed from needs_work to needs_review

comment:91 Changed 3 years ago by vbraun

You can use this:

            sage: hash(SR(19.23))
            -1458111714  # 32-bit
            2836855582   # 64-bit

comment:92 Changed 3 years ago by git

  • Commit changed from d5fed29f178c22211c76f0b5875b4d1329573c80 to d94b19ec7f8dd6ef619bb2a47c5233a9718aa6cf

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

d94b19eChanged doctest to allow 32/64 bit output.

comment:93 Changed 3 years ago by mmasdeu

I have followed Volker's suggestion. I leave it as needs_review...

comment:94 Changed 3 years ago by wuthrich

  • Status changed from needs_review to positive_review

To me all tests still pass and I can't check 32-bit 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 3 years ago by git

  • Commit changed from d94b19ec7f8dd6ef619bb2a47c5233a9718aa6cf to d11150c160981e911468854be4a493d44c56b6d9
  • 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:

d11150cMoved an auxiliary function where it was used.

comment:96 Changed 3 years ago by mmasdeu

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 3 years ago by wuthrich

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 3 years ago by git

  • Commit changed from d11150c160981e911468854be4a493d44c56b6d9 to 31a3e87ceeb7ba39b884ef22fc7adffb18c841fc

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

31a3e87Added missing doctest.

comment:99 Changed 3 years ago by mmasdeu

I fixed the missing doctest in padics.py as well.


New commits:

31a3e87Added missing doctest.

comment:100 Changed 3 years ago by wuthrich

  • Status changed from needs_review to positive_review

tests pass.

comment:101 Changed 3 years ago by vbraun

  • Status changed from positive_review to 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 13628--13628
[docpdf] \T1/ptm/m/it/10 char-ac-ter=None\T1/ptm/m/n/10 ,
[docpdf] 
[docpdf] Underfull \hbox (badness 10000) in paragraph at lines 13628--13628
[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 13629--13630
[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 14890--14891
[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 3 years ago by wuthrich

  • Branch changed from u/mmasdeu/812-pollackstevens to u/wuthrich/ticket/812
  • Commit changed from 31a3e87ceeb7ba39b884ef22fc7adffb18c841fc to 44c321f58f42ec0b11fbe8674545687b3562e73a

pdf documentation now build for me.


Last 10 new commits:

afeb1a1Trac #812: Removed unneeded imports.
3a54bd9Trac #812: Changed "print" to "string" in some of the docstrings to avoid bug in patchbot.
32fa647Trac 812: changed signature of ModuleElement.
cef9a4aMerge branch 'develop' into 812-chris2
4374c5bTrac #812: finished adapting the signatures.
d5fed29Marked some doctests with random output. Fixed problem with magma maximal order.
d94b19eChanged doctest to allow 32/64 bit output.
d11150cMoved an auxiliary function where it was used.
31a3e87Added missing doctest.
44c321ftrac 812: docstring changes for pdf

comment:103 Changed 3 years ago by wuthrich

  • Status changed from needs_work to positive_review

comment:104 Changed 3 years ago by vbraun

  • Branch changed from u/wuthrich/ticket/812 to 44c321f58f42ec0b11fbe8674545687b3562e73a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.