Opened 8 years ago
Closed 5 years ago
#11005 closed enhancement (fixed)
Update Simon's GP scripts
Reported by:  cremona  Owned by:  cremona 

Priority:  major  Milestone:  sage6.2 
Component:  elliptic curves  Keywords:  simon_two_descent spkg 
Cc:  was, pbruin  Merged in:  
Authors:  Peter Bruin  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  24b6fe9 (Commits)  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 8 years ago by
comment:1 Changed 8 years ago by
 Cc was added
 Description modified (diff)
 Status changed from new to needs_review
comment:2 Changed 8 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 8 years ago by
 Status changed from needs_review to needs_work
Not yet ready for review: still testing.
comment:4 Changed 8 years ago by
 Status changed from needs_work to needs_review
comment:5 Changed 8 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 8 years ago by
comment:6 Changed 8 years ago by
 Description modified (diff)
comment:7 Changed 8 years ago by
 Keywords simon added
 Status changed from needs_review to 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 8 years ago by
 Status changed from needs_info to 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 8 years ago by
 Status changed from needs_review to needs_work
comment:10 Changed 8 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 8 years ago by
 Dependencies set to #11230, #11234, #11130
comment:12 followup: ↓ 13 Changed 8 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 in reply to: ↑ 12 ; followup: ↓ 14 Changed 8 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 in reply to: ↑ 13 Changed 8 years ago by
 Milestone changed from sage5.0 to 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 8 years ago by
 Description modified (diff)
comment:16 Changed 8 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 8 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:18 Changed 8 years ago by
what are the work issues for that ticket?
Paul
comment:19 Changed 8 years ago by
 Work issues set to 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 7 years ago by
comment:20 Changed 7 years ago by
This patch works in the sense that most tests pass. There is, though, still work to be done.
comment:21 Changed 7 years ago by
 Description modified (diff)
comment:22 Changed 7 years ago by
I am now working on correcting the doctests (so not ready for review yet).
comment:23 Changed 6 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:24 Changed 6 years ago by
 Keywords simon_two_descent added; simon removed
comment:25 Changed 5 years ago by
 Cc pbruin added
comment:26 Changed 5 years ago by
 Description modified (diff)
comment:27 Changed 5 years ago by
 Description modified (diff)
comment:28 Changed 5 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:29 Changed 5 years ago by
 Description modified (diff)
 Summary changed from Update Simon's GP scripts and convert to use via gp2c to Update Simon's GP scripts
comment:30 Changed 5 years ago by
 Description modified (diff)
 Work issues Update to work with Simon's latest scripts deleted
comment:31 Changed 5 years ago by
 Description modified (diff)
comment:32 Changed 5 years ago by
 Branch set to u/pbruin/11005Simon_update
 Commit set to 24b6fe94424561dce5e9f97e656c82c486aa6c78
 Status changed from needs_work to 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 5 years ago by
 Description modified (diff)
comment:34 Changed 5 years ago by
 Reviewers set to Jeroen Demeyer
 Status changed from needs_review to positive_review
comment:35 Changed 5 years ago by
 Branch changed from u/pbruin/11005Simon_update to 24b6fe94424561dce5e9f97e656c82c486aa6c78
 Resolution set to fixed
 Status changed from positive_review to closed
comment:36 Changed 5 years ago by
 Commit 24b6fe94424561dce5e9f97e656c82c486aa6c78 deleted
 Resolution fixed deleted
 Status changed from closed to new
comment:37 Changed 5 years ago by
 Dependencies changed from #11230, #11234, #11130 to #11230, #11234, #11130, #15483
 Status changed from new to needs_review
comment:38 Changed 5 years ago by
The pbori.pyx
failure is from #15755.
comment:39 followup: ↓ 42 Changed 5 years ago by
comment:40 Changed 5 years ago by
 Branch changed from 24b6fe94424561dce5e9f97e656c82c486aa6c78 to u/pbruin/11005Simon_update
 Commit set to 24b6fe94424561dce5e9f97e656c82c486aa6c78
New commits:
24b6fe9  Update Denis Simon's GP scripts to versions of 06/04/2011 (ell.gp)

comment:41 Changed 5 years ago by
 Status changed from needs_review to needs_work
comment:42 in reply to: ↑ 39 Changed 5 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 5 years ago by
 Status changed from needs_work to positive_review
comment:44 Changed 5 years ago by
 Branch changed from u/pbruin/11005Simon_update to 24b6fe94424561dce5e9f97e656c82c486aa6c78
 Resolution set to fixed
 Status changed from positive_review to closed
applies to 4.7.alpha1