Opened 12 years ago
Closed 9 years ago
#11005 closed enhancement (fixed)
Update Simon's GP scripts
Reported by:  John Cremona  Owned by:  John Cremona 

Priority:  major  Milestone:  sage6.2 
Component:  elliptic curves  Keywords:  simon_two_descent spkg 
Cc:  William Stein, Peter Bruin  Merged in:  
Authors:  Peter Bruin  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  24b6fe9 (Commits, GitHub, GitLab)  Commit:  24b6fe94424561dce5e9f97e656c82c486aa6c78 
Dependencies:  #11230, #11234, #11130, #15483  Stopgaps: 
Description (last modified by )
[See #15608 for a list of open simon_two_descent tickets]
Denis Simon has new versions of his scripts (see http://www.math.unicaen.fr/~simon/), these should be updated (see src/ext/pari/simon
).
Attachments (5)
Change History (49)
Changed 12 years ago by
Attachment:  trac_11005simonsage.patch added 

comment:1 Changed 12 years ago by
Cc:  William Stein added 

Description:  modified (diff) 
Status:  new → needs_review 
comment:2 Changed 12 years ago by
Note to reviewers. It is very likely that you will find elliptic curves where the scripts do not work (I have some of my own). That is *not* a reason to reject this enhancement. The scripts are not perfect, but they are all we have over number fields! As a result of this ticket, we get both updated version of the scripts (with some bugfixes and enhancements) and also faster execution. But not perfection.
comment:3 Changed 12 years ago by
Status:  needs_review → needs_work 

Not yet ready for review: still testing.
comment:4 Changed 12 years ago by
Status:  needs_work → needs_review 

comment:5 Changed 12 years ago by
Description:  modified (diff) 

I created a new spkg and I updated the way to update import the library, removing one GPscript from the spkg. I can give this a positive review, but before someone needs to test this on Mac at least.
Changed 12 years ago by
Attachment:  trac11005simonsage2.patch added 

comment:6 Changed 12 years ago by
Description:  modified (diff) 

comment:7 Changed 12 years ago by
Authors:  John Cremona → John Cremona, Martin Raum 

Keywords:  simon added 
Status:  needs_review → needs_info 
Martin, the spkg at http://sage.math.washington.edu/home/mraum/simon20110321.spkg seems to be identical to my original one, which cannot be right.
comment:8 Changed 12 years ago by
Status:  needs_info → needs_review 

I checked this, but indeed this is the right package. The point was to update the spkginstall script. The rest of the spkg, I think is perfect. Well, we should incorporate the new scripts that you received by Simon.
comment:9 Changed 12 years ago by
Status:  needs_review → needs_work 

comment:10 Changed 12 years ago by
Keywords:  spkg added 

Note that adding a new standard spkg to Sage would at least require changing some files in SAGE_ROOT, in particular spkg/install
and spkg/standard/deps
.
comment:11 Changed 12 years ago by
Dependencies:  → #11230, #11234, #11130 

comment:12 followup: 13 Changed 12 years ago by
I don't see much reason for this a separate spkg. Given that it is essentially one .c file, could we not put this into the main Sage library?
comment:13 followup: 14 Changed 12 years ago by
Replying to jdemeyer:
I don't see much reason for this a separate spkg. Given that it is essentially one .c file, could we not put this into the main Sage library?
I think that is a great idea. Putting it into an spkg seemed simpler as a way to work on it and test it, but I would be very happy to put it into the library. I guess that can be done by applying patches to the right places (the usual Sage library, but where else? ) and then would "sage b" be enough to include it while testing?
comment:14 Changed 12 years ago by
Milestone:  sage5.0 → sage4.7.1 

Replying to cremona:
I think that is a great idea. Putting it into an spkg seemed simpler as a way to work on it and test it, but I would be very happy to put it into the library. I guess that can be done by applying patches to the right places (the usual Sage library, but where else? ) and then would "sage b" be enough to include it while testing?
You would need to edit also module_list.py
. But I haven't given this idea much thought for now, I plan to work on this after 4.7 has been released.
comment:15 Changed 12 years ago by
Description:  modified (diff) 

comment:16 Changed 11 years ago by
Is this the main block to getting pari2.5.0 in (I am aware there are also issues with lcalc on OS X but they seem to be actively worked on).
The description needs editing, it talks about two patches but there are three given. Plus point (1) doesn't make complete sense (it looks like some writing I could have done at 2am and forgot about).
comment:17 Changed 11 years ago by
I'll look at this again  it was a lot of work (by Martin Raum, Henri Cohen and me) getting this to work at Sage Days in March, and a great disappointment that it then stalled.
But I don't think this affects the 2.5.0 issue; rather the contrary, I did not want to put more work into this one until the pari version was updated.
comment:19 Changed 11 years ago by
Work issues:  → Update to work with Simon's latest scripts 

This is the ticket I mentioned in my recent posting to the other ticket #9322.
I think I was waiting for Jeroen Demeyer who 5 months ago said he would look at it after 4.7 was released, but clearly had other things to do (as have I). I'll add it to my list of things to do.
One issue I do remember is that to make Simon's scripts work here we did a lot of editing of them (to make them correctly compile with gp2c), and told Simon what we had done, then as a result he made a new version which was already gp2ccompatible. So then someone (probably me) has to compare the versions so as to merge our changes, in effect. For example, one set of changes I made was to have all the toplevel functions take as parameters all the parameters which Simon currently sets using global gp variables; this made his functions a lot cleaner to use.
Changed 11 years ago by
Attachment:  trac_11005simonsage3.patch added 

comment:20 Changed 11 years ago by
This patch works in the sense that most tests pass. There is, though, still work to be done.
comment:21 Changed 11 years ago by
Description:  modified (diff) 

comment:22 Changed 11 years ago by
I am now working on correcting the doctests (so not ready for review yet).
comment:23 Changed 9 years ago by
Milestone:  sage5.11 → sage5.12 

comment:24 Changed 9 years ago by
Keywords:  simon_two_descent added; simon removed 

comment:25 Changed 9 years ago by
Cc:  Peter Bruin added 

comment:26 Changed 9 years ago by
Description:  modified (diff) 

comment:27 Changed 9 years ago by
Description:  modified (diff) 

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

comment:29 Changed 9 years ago by
Description:  modified (diff) 

Summary:  Update Simon's GP scripts and convert to use via gp2c → Update Simon's GP scripts 
comment:30 Changed 9 years ago by
Description:  modified (diff) 

Work issues:  Update to work with Simon's latest scripts 
comment:31 Changed 9 years ago by
Description:  modified (diff) 

comment:32 Changed 9 years ago by
Authors:  John Cremona, Martin Raum → Peter Bruin 

Branch:  → u/pbruin/11005Simon_update 
Commit:  → 24b6fe94424561dce5e9f97e656c82c486aa6c78 
Status:  needs_work → needs_review 
This branch updates Denis Simon's GP scripts to versions 06/04/2011 (ell.gp
) resp. 13/01/2014 (ellQ.gp
, ellcommon.gp
[new], qfsolve.gp
, resultant3.gp
). The scripts themselves were copied unchanged from Simon's website (http://www.math.unicaen.fr/~simon/
).
Summary of the other changes:
 change
Qfsolve
andQfparam
toqfsolve
andqfparam
 remove one doctest in
gp_simon.py
that is also inell_rational_field.py
 remove
# random output
inell_number_field.py
; if necessary we can addset_random_seed(0)
before that, but I got the same output every time  doctest fixes due to changed test output
 minor documentation fixes
Of course the real work in this ticket was done (indirectly) by Denis Simon, and I just made the necessary fixes to the Sage library, but it seems to me that the "Author" field is normally used for ticket authors rather than authors of any "upstream" code; do correct me if I'm wrong.
Jeroen has opened #15809 for the gp2c
conversion; hopefully John and Martin's hard work can somehow be transplanted there!
comment:33 Changed 9 years ago by
Description:  modified (diff) 

comment:34 Changed 9 years ago by
Reviewers:  → Jeroen Demeyer 

Status:  needs_review → positive_review 
comment:35 Changed 9 years ago by
Branch:  u/pbruin/11005Simon_update → 24b6fe94424561dce5e9f97e656c82c486aa6c78 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:36 Changed 9 years ago by
Commit:  24b6fe94424561dce5e9f97e656c82c486aa6c78 

Resolution:  fixed 
Status:  closed → new 
comment:37 Changed 9 years ago by
Dependencies:  #11230, #11234, #11130 → #11230, #11234, #11130, #15483 

Status:  new → needs_review 
comment:39 followup: 42 Changed 9 years ago by
comment:40 Changed 9 years ago by
Branch:  24b6fe94424561dce5e9f97e656c82c486aa6c78 → u/pbruin/11005Simon_update 

Commit:  → 24b6fe94424561dce5e9f97e656c82c486aa6c78 
New commits:
24b6fe9  Update Denis Simon's GP scripts to versions of 06/04/2011 (ell.gp)

comment:41 Changed 9 years ago by
Status:  needs_review → needs_work 

comment:42 Changed 9 years ago by
Replying to jdemeyer:
Why the dependency on #15483?
Because #15483 is the obvious fix for the problem from comment:36.
Now both these tickets depend on eachother. I would prefer to fix #11005 first (using
# 32bit
/# 64bit
) and then do #15483 on top of this.
I thought Git was supposed to make things like this easy. I knew that I made the two tickets depend on each other, of course, and agree that circular dependencies are in abstracto not very nice. However, in this case, what looks like a circular dependency is just a way of saying that the tickets should be merged together; the structure of the Git branches is still linear and easy to understand. Moreover, Git will automatically give the same result independently of the ordering in which the two branches are merged. I don't feel like spending any time on making each of the two tickets pass doctests separately.
I also realise you don't like the patch at #15483, so I will make a bit more propaganda for it.
comment:43 Changed 9 years ago by
Status:  needs_work → positive_review 

comment:44 Changed 9 years ago by
Branch:  u/pbruin/11005Simon_update → 24b6fe94424561dce5e9f97e656c82c486aa6c78 

Resolution:  → fixed 
Status:  positive_review → closed 
applies to 4.7.alpha1