Opened 10 years ago

Last modified 6 months ago

#11024 needs_info enhancement

Update Dokchitser calculator to compiled version

Reported by: mraum Owned by: craigcitro
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: modular forms Keywords: dokchitser
Cc: cremona, rishi, pbruin Merged in:
Authors: Martin Raum, Henri Cohen Reviewers: John Cremona
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #11017 Stopgaps:

Status badges

Description (last modified by chapoton)

Cohen provided a C-version of Dokchitser's script. This wraps that script and cleans up the script. There were several issues present and doctests for the elliptic curves have been adapted to correctly set the maximal imaginary value.

Apply:

  1. trac-11024-dokchitser.patch
  2. trac-11024-dokchitser-2.patch
  3. trac-11024-dokchitser-bordeaux.patch
  4. http://sage.math.washington.edu/home/mraum/dokchitser-20110325.spkg

Attachments (4)

trac-11024-dokchitser.patch (43.3 KB) - added by mraum 10 years ago.
dokchitser.spkg (15.3 KB) - added by mraum 10 years ago.
trac-11024-dokchitser-2.patch (1.7 KB) - added by mraum 10 years ago.
Something went wrong with diff, this patch is better.
trac-11024-dokchitser-bordeaux.patch (520 bytes) - added by mraum 10 years ago.

Download all attachments as: .zip

Change History (36)

Changed 10 years ago by mraum

Changed 10 years ago by mraum

comment:1 Changed 10 years ago by mraum

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

comment:2 Changed 10 years ago by cremona

  • Cc cremona added

comment:3 Changed 10 years ago by cremona

  • Reviewers set to John Cremona

The patch and spkg installed OK (on 63-bit ubuntu) and I am now testing.

  • The spkg needs a version number, so use the date (e.g. dokchitser-20110325.spkg)
  • There needs to be an SPKG.txt file
  • The spkg needs to be under mercurial version control (but *not* the src/ subdirectory).
  • The spkg-install looks good!

These are trivialities, but necessary!

comment:4 Changed 10 years ago by mraum

  • Description modified (diff)

Updated spkg available.

comment:5 Changed 10 years ago by cremona

  • Status changed from needs_review to needs_work

I'll look at the new spkg. Meanwhile testing revealed one small problem:

sage -t "devel/sage-main/doc/en/bordeaux_2008/elliptic_curves.rst"

comment:6 Changed 10 years ago by cremona

New spkg: SPKG.txt seems to refer to a different spkg!!

Also I suggest adding .hgignore to list src/

comment:7 Changed 10 years ago by was

  • Authors changed from Martin Raum to Martin Raum, Henri Cohen

comment:8 Changed 10 years ago by mraum

A tiny change to the Dokchitser wrapper. The old way of finding the library was kind of dump. Also the spkg now contains the .hgignore as you suggested.

Changed 10 years ago by mraum

Something went wrong with diff, this patch is better.

Changed 10 years ago by mraum

comment:9 Changed 10 years ago by mraum

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Fix the doctest in the Bordeaux notes. We need to set the maximal imaginary value correctly.

comment:10 Changed 10 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Martin: patches should be created using hg export tip instead of hg diff. If you use queues, be sure to set a meaningful commit message using hg qrefresh -e. The commit message should contain the ticket number on the first line.

comment:11 Changed 10 years ago by rishi

  • Cc rishi added

comment:12 Changed 10 years ago by rishi

From my small tests, the patch works good, and it eliminated the bug which I saw yesterday. I would second Jeroen's request that there should be a meaningful commit message. Right now it is set of diffs.

I would have done it right now, but it will be hard for me to write an appropriate commit message without essentially reviewing it. I will do it next weekend if no one has done it by then.

comment:13 Changed 10 years ago by jdemeyer

If you are adding a new spkg to Sage, you should also update various files in SAGE_ROOT, in particular spkg/install and spkg/standard/deps.

comment:14 Changed 10 years ago by mraum

Jeroen added a comment to #11005, stating that it is probably better to add the single c-file to the Sage library. We probably also want to do this for the Dokchitser calculator.

I hope to have time for this soon.

comment:15 Changed 8 years ago by chapoton

  • Keywords dokchitser added

comment:16 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:17 Changed 8 years ago by chapoton

  • Dependencies set to #11017
  • Description modified (diff)

comment:18 Changed 8 years ago by cremona

Thanks a lot for reviving this!

If you fancy doing the same with the Simon 2-descent scripts that would be fantastic. Both Martin Raum and I tried to do that a couple of years ago but failed (and I don't remember why). Again, Henri Cohen and I first spent a while cleaning up the scripts to avoid global variables and such things. I would certainly be willing to work jointly on that as I know his code pretty well. There are also plans within the pari developers group to implement all that in the pari library, but I don't know how far that has got.

comment:19 Changed 8 years ago by chapoton

Hello,

Just to be clear, I have not yet done anything serious for this ticket.

It seems that the current spkg (as given by the link) needs to be updated, because the file computel5.gp.c contains three calls to the function gtoser with a wrong number of arguments.

comment:20 Changed 8 years ago by cremona

Karim Belabas says "This is scheduled for implementation in PARI core." I don't know how far off that is, but once it has happened we will not need the gp script but instead additions to the pari library interface.

comment:21 Changed 7 years ago by pbruin

  • Cc pbruin added

comment:22 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:23 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:24 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:26 Changed 3 years ago by chapoton

see #26098 for another tentative

comment:27 Changed 13 months ago by pbruin

  • Milestone changed from sage-6.4 to sage-duplicate/invalid/wontfix
  • Status changed from needs_work to needs_review

It looks like this is no longer needed after #26098. Can someone who has worked on this ticket confirm this?

comment:28 follow-up: Changed 13 months ago by cremona

I agree with that (see my comment 20 above).

comment:29 in reply to: ↑ 28 Changed 13 months ago by pbruin

  • Status changed from needs_review to positive_review

Replying to cremona:

I agree with that (see my comment 20 above).

OK, thanks.

comment:30 follow-up: Changed 10 months ago by chapoton

Either one decides to remove the Dokschitser scripts and rely only on pari itself, or this ticket still makes sense. So far, both implementations coexist.

comment:31 in reply to: ↑ 30 Changed 10 months ago by cremona

Replying to chapoton:

Either one decides to remove the Dokschitser scripts and rely only on pari itself, or this ticket still makes sense. So far, both implementations coexist.

Please let us remove the Dokchitser script. It is very old and certainly not supported by its author who mvoed over entirely to Magma for these computations.

I suppose some deprecation should be put in place first?

comment:32 Changed 6 months ago by slelievre

  • Status changed from positive_review to needs_info
Note: See TracTickets for help on using tickets.