Opened 11 years ago
Last modified 13 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: |
Description (last modified by )
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:
Attachments (4)
Change History (36)
Changed 11 years ago by
Changed 11 years ago by
comment:1 Changed 11 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 11 years ago by
- Cc cremona added
comment:3 Changed 11 years ago by
- Reviewers set to John Cremona
comment:5 Changed 11 years ago by
- 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 11 years ago by
New spkg: SPKG.txt seems to refer to a different spkg!!
Also I suggest adding .hgignore to list src/
comment:7 Changed 11 years ago by
comment:8 Changed 11 years ago by
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 11 years ago by
comment:9 Changed 11 years ago by
- 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 11 years ago by
- 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 11 years ago by
- Cc rishi added
comment:12 Changed 11 years ago by
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 11 years ago by
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
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
- Keywords dokchitser added
comment:16 Changed 8 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:17 Changed 8 years ago by
- Dependencies set to #11017
- Description modified (diff)
comment:18 Changed 8 years ago by
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
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
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 8 years ago by
- Cc pbruin added
comment:22 Changed 8 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:23 Changed 7 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:24 Changed 7 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:25 Changed 3 years ago by
comment:26 Changed 3 years ago by
see #26098 for another tentative
comment:27 Changed 19 months ago by
- 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: ↓ 29 Changed 19 months ago by
I agree with that (see my comment 20 above).
comment:29 in reply to: ↑ 28 Changed 19 months ago by
- Status changed from needs_review to positive_review
comment:30 follow-up: ↓ 31 Changed 16 months ago by
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 16 months ago by
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 13 months ago by
- Status changed from positive_review to needs_info
The patch and spkg installed OK (on 63-bit ubuntu) and I am now testing.
These are trivialities, but necessary!