Opened 9 years ago

Closed 6 years ago

#11005 closed enhancement (fixed)

Update Simon's GP scripts

Reported by: cremona Owned by: cremona
Priority: major Milestone: sage-6.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 pbruin)

[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)

trac_11005-simon-sage.patch (6.2 KB) - added by cremona 9 years ago.
applies to 4.7.alpha1
trac_11005-simon-extcode.patch (162.5 KB) - added by cremona 9 years ago.
applies to 4.7.alpha1
simon-20110321.spkg (94.1 KB) - added by cremona 9 years ago.
New spkg
trac-11005-simon-sage-2.patch (15.7 KB) - added by mraum 9 years ago.
trac_11005-simon-sage-3.patch (358.6 KB) - added by mraum 8 years ago.

Download all attachments as: .zip

Change History (49)

Changed 9 years ago by cremona

applies to 4.7.alpha1

Changed 9 years ago by cremona

applies to 4.7.alpha1

comment:1 Changed 9 years ago by cremona

  • Cc was added
  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 9 years ago by cremona

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 bug-fixes and enhancements) and also faster execution. But not perfection.

comment:3 Changed 9 years ago by cremona

  • Status changed from needs_review to needs_work

Not yet ready for review: still testing.

Changed 9 years ago by cremona

New spkg

comment:4 Changed 9 years ago by cremona

  • Status changed from needs_work to needs_review

comment:5 Changed 9 years ago by mraum

  • Description modified (diff)

I created a new spkg and I updated the way to update import the library, removing one GP-script from the spkg. I can give this a positive review, but before someone needs to test this on Mac at least.

Changed 9 years ago by mraum

comment:6 Changed 9 years ago by mraum

  • Description modified (diff)

comment:7 Changed 9 years ago by cremona

  • Authors changed from John Cremona to John Cremona, Martin Raum
  • Keywords simon added
  • Status changed from needs_review to needs_info

Martin, the spkg at http://sage.math.washington.edu/home/mraum/simon-20110321.spkg seems to be identical to my original one, which cannot be right.

comment:8 Changed 9 years ago by mraum

  • Status changed from needs_info to needs_review

I checked this, but indeed this is the right package. The point was to update the spkg-install 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 9 years ago by mraum

  • Status changed from needs_review to needs_work

comment:10 Changed 9 years ago by jdemeyer

  • 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 9 years ago by jdemeyer

  • Dependencies set to #11230, #11234, #11130

comment:12 follow-up: Changed 9 years ago by 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?

comment:13 in reply to: ↑ 12 ; follow-up: Changed 9 years ago by cremona

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 9 years ago by jdemeyer

  • Milestone changed from sage-5.0 to sage-4.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 9 years ago by jdemeyer

  • Description modified (diff)

comment:16 Changed 8 years ago by fbissey

Is this the main block to getting pari-2.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 cremona

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 zimmerma

what are the work issues for that ticket?

Paul

comment:19 Changed 8 years ago by cremona

  • 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 gp2c-compatible. 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 top-level 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 8 years ago by mraum

comment:20 Changed 8 years ago by mraum

This patch works in the sense that most tests pass. There is, though, still work to be done.

comment:21 Changed 8 years ago by cremona

  • Description modified (diff)

comment:22 Changed 8 years ago by cremona

I am now working on correcting the doctests (so not ready for review yet).

comment:23 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:24 Changed 6 years ago by chapoton

  • Keywords simon_two_descent added; simon removed

comment:25 Changed 6 years ago by pbruin

  • Cc pbruin added

comment:26 Changed 6 years ago by pbruin

  • Description modified (diff)

comment:27 Changed 6 years ago by cremona

  • Description modified (diff)

comment:28 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:29 Changed 6 years ago by jdemeyer

  • 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 6 years ago by jdemeyer

  • Description modified (diff)
  • Work issues Update to work with Simon's latest scripts deleted

comment:31 Changed 6 years ago by jdemeyer

  • Description modified (diff)

comment:32 Changed 6 years ago by pbruin

  • Authors changed from John Cremona, Martin Raum to Peter Bruin
  • Branch set to u/pbruin/11005-Simon_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 and Qfparam to qfsolve and qfparam
  • remove one doctest in gp_simon.py that is also in ell_rational_field.py
  • remove # random output in ell_number_field.py; if necessary we can add set_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 6 years ago by pbruin

  • Description modified (diff)

comment:34 Changed 6 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:35 Changed 6 years ago by vbraun

  • Branch changed from u/pbruin/11005-Simon_update to 24b6fe94424561dce5e9f97e656c82c486aa6c78
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:37 Changed 6 years ago by pbruin

  • Dependencies changed from #11230, #11234, #11130 to #11230, #11234, #11130, #15483
  • Status changed from new to needs_review

comment:38 Changed 6 years ago by jdemeyer

The pbori.pyx failure is from #15755.

Last edited 6 years ago by jdemeyer (previous) (diff)

comment:39 follow-up: Changed 6 years ago by jdemeyer

Why the dependency on #15483? Now both these tickets depend on eachother. I would prefer to fix #11005 first (using # 32-bit/# 64-bit) and then do #15483 on top of this.

comment:40 Changed 6 years ago by jdemeyer

  • Branch changed from 24b6fe94424561dce5e9f97e656c82c486aa6c78 to u/pbruin/11005-Simon_update
  • Commit set to 24b6fe94424561dce5e9f97e656c82c486aa6c78

New commits:

24b6fe9Update Denis Simon's GP scripts to versions of 06/04/2011 (ell.gp)

comment:41 Changed 6 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:42 in reply to: ↑ 39 Changed 6 years ago by pbruin

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 # 32-bit/# 64-bit) 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 6 years ago by jdemeyer

  • Status changed from needs_work to positive_review

comment:44 Changed 6 years ago by vbraun

  • Branch changed from u/pbruin/11005-Simon_update to 24b6fe94424561dce5e9f97e656c82c486aa6c78
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.