Opened 13 years ago
Closed 8 years ago
#6666 closed enhancement (fixed)
Implement analytic modular symbols for elliptic curves
Reported by:  William Stein  Owned by:  William Stein 

Priority:  minor  Milestone:  sage6.4 
Component:  modular forms  Keywords:  period, modular symbol 
Cc:  Peter Bruin, John Cremona  Merged in:  
Authors:  William Stein, Peter Bruin  Reviewers:  Frédéric Chapoton, Peter Bruin 
Report Upstream:  N/A  Work issues:  
Branch:  dcaefdc (Commits, GitHub, GitLab)  Commit:  dcaefdc03a500b6dcc9e3d683e7dc4bfd7e8a685 
Dependencies:  Stopgaps: 
Description (last modified by )
Attachments (2)
Change History (26)
Changed 13 years ago by
Attachment:  trac_6666part1.patch added 

comment:1 Changed 9 years ago by
Component:  number theory → modular forms 

Report Upstream:  → N/A 
comment:2 Changed 9 years ago by
Milestone:  sage5.11 → sage5.12 

comment:3 Changed 9 years ago by
comment:4 Changed 9 years ago by
Keywords:  period modular symbol added 

Status:  needs_work → needs_info 
Summary:  [with patch] implement analytic modular symbols algorithm and cusp transformation matrix → implement analytic modular symbols algorithm and cusp transformation matrix 
comment:5 Changed 9 years ago by
Frédéric Chapoton  whoever you are  I'm extremely happy seeing all the work you're doing on modular forms related functionality in Sage!!!!!!
+1000
 William Stein
comment:6 followup: 7 Changed 9 years ago by
Thanks William. I am in algebra and combinatorics, not a number theorist, but I am trying to help nevertheless.
for the patchbot:
apply trac_6666rebased5.12.patch
comment:7 Changed 9 years ago by
Replying to chapoton:
Thanks William. I am in algebra and combinatorics, not a number theorist, but I am trying to help nevertheless.
Thanks. I'm in number theory, not algebra/combinatorics, but I hope Sage has been helpful to people in algebra/combinatorics :)
comment:8 Changed 9 years ago by
Status:  needs_info → needs_review 

ok, the bot has turned green. Needs review
comment:9 Changed 9 years ago by
new patch, rebased on 5.13.beta1
apply trac_6666rebased5.12.patch
comment:10 Changed 9 years ago by
Description:  modified (diff) 

So there is numerical noise. Could somebody remind me what is the proper way to handle that ?
Use ...
or use # rel tol
or use # abs tol
?
comment:11 Changed 9 years ago by
new patch, with numerical tolerance
apply trac_6666rebased5.12.patch
Changed 9 years ago by
Attachment:  trac_6666rebased5.12.patch added 

comment:14 Changed 9 years ago by
Branch:  → u/chapoton/6666 

Commit:  → 1986947ca4929007eaa5bdc06649ce79a93a41c3 
comment:15 Changed 9 years ago by
Commit:  1986947ca4929007eaa5bdc06649ce79a93a41c3 → 2e06ddb568811cd552cc02695d2e8f87792ca9d8 

Branch pushed to git repo; I updated commit sha1. New commits:
2e06ddb  trac #6666  implement analytic modular symbols algorithm and cusp transformation matrix

comment:16 Changed 9 years ago by
Milestone:  sage6.1 → sage6.2 

comment:17 Changed 9 years ago by
Milestone:  sage6.2 → sage6.3 

comment:18 Changed 8 years ago by
Milestone:  sage6.3 → sage6.4 

comment:19 Changed 8 years ago by
Cc:  Peter Bruin added 

comment:20 Changed 8 years ago by
Commit:  2e06ddb568811cd552cc02695d2e8f87792ca9d8 → 91eb78f5c9d51048d4a9d5d36964719f4dc740b3 

comment:21 Changed 8 years ago by
Authors:  → William Stein, Peter Bruin 

Branch:  u/chapoton/6666 → u/pbruin/6666modular_symbol_numerical 
Cc:  John Cremona added 
Commit:  91eb78f5c9d51048d4a9d5d36964719f4dc740b3 → b50bec1fbedb9ca734c47a5ab9ccb6ffee4bfa9a 
Reviewers:  → Frédéric Chapoton, Peter Bruin 
Summary:  implement analytic modular symbols algorithm and cusp transformation matrix → Implement analytic modular symbols for elliptic curves 
comment:22 followup: 23 Changed 8 years ago by
Branch:  u/pbruin/6666modular_symbol_numerical → public/6666 

Commit:  b50bec1fbedb9ca734c47a5ab9ccb6ffee4bfa9a → dcaefdc03a500b6dcc9e3d683e7dc4bfd7e8a685 
Status:  needs_review → positive_review 
comment:23 Changed 8 years ago by
Replying to chapoton:
Looks good to me.
Thanks for the review!
I allowed myself a few minor changes.
I'm not disputing your changes to the whitespace here, but note that PEP 8 does not say that there should be spaces around all operators, only the relational ones. From https://www.python.org/dev/peps/pep0008/:
If operators with different priorities are used, consider adding whitespace around the operators with the lowest priority(ies). Use your own judgment; however, never use more than one space, and always have the same amount of whitespace on both sides of a binary operator. Yes:
i = i + 1 submitted += 1 x = x*2  1 hypot2 = x*x + y*y c = (a+b) * (ab)No:
i=i+1 submitted +=1 x = x * 2  1 hypot2 = x * x + y * y c = (a + b) * (a  b)
Actually, in the case of c = ...
, I would personally prefer the "no" option or even (a + b)*(a  b)
, which is closer to standard mathematical typesetting, but in any case this is a matter of taste.
comment:24 Changed 8 years ago by
Branch:  public/6666 → dcaefdc03a500b6dcc9e3d683e7dc4bfd7e8a685 

Resolution:  → fixed 
Status:  positive_review → closed 
Here is a rebased patch.
apply trac_6666rebased5.12.patch