Opened 12 years ago
Closed 12 years ago
#8496 closed enhancement (fixed)
Implement canonical heights for elliptic curves over number fields
Reported by: | robertwb | Owned by: | was |
---|---|---|---|
Priority: | major | Milestone: | sage-4.4 |
Component: | elliptic curves | Keywords: | |
Cc: | Merged in: | sage-4.4.alpha1 | |
Authors: | Robert Bradshaw, Chris Wuthrich | Reviewers: | Chris Wuthrich, John Cremona |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
Attachments (4)
Change History (21)
Changed 12 years ago by
comment:1 Changed 12 years ago by
- Status changed from new to needs_review
comment:2 Changed 12 years ago by
comment:3 Changed 12 years ago by
Excellent -- I will look at it too. I have implemented this more than once, so know the algorithm well.
Any reason not to use ticket #360 which has been patiently waiting for 3 years?
comment:4 Changed 12 years ago by
The patch did not apply to 4.3.4.alpha1.
Hunk #4 FAILED at 1687
A rebase is needed.
comment:5 Changed 12 years ago by
Hmm... #360 looks like it's also about implementing height bounds as well, which I also plan to do if no one beats me to it, but don't want to wait up to get this in.
I'll have to get ahold of an alpha to try and rebase it (and document the normalization choice).
comment:6 Changed 12 years ago by
- Owner changed from cremona to was
I noticed in the docstrings that you specify INPUT variables like this:
1824 INPUT:: 1825 v - a non-archimedean place of the base field of the curve, 1826 or None, in which case the total nonarchimedian contribution 1827 is returned 1828 1829 prec - working precision, or None in which case the height 1830 is returned symbolically
I don't think this is so nice in Sphinx, since it is just preformated. The vast majority of docstrings are formatted like as a list:
1824 INPUT: 1825 - ``v`` - a non-archimedean place of the base field of the curve, 1826 or None, in which case the total nonarchimedian contribution 1827 is returned 1828 1829 - ``prec`` - working precision, or None in which case the height 1830 is returned symbolically
Note that:
- only one colon after "INPUT"
- a dash at the start of each line
- the indentation is two spaces after the dash.
comment:7 follow-up: ↓ 8 Changed 12 years ago by
Is there a reason the 0 is returned in QQ ?
I will later modify this, because I will need the exp() of the local heights for non-archimedean primes as an element in QQ, if I want to implement the p-adic heights over number fields.
comment:8 in reply to: ↑ 7 ; follow-up: ↓ 9 Changed 12 years ago by
Replying to wuthrich:
Is there a reason the 0 is returned in QQ ?
For consistency with the original code, and for speed (not that it should matter much).
I will later modify this, because I will need the exp() of the local heights for non-archimedean primes as an element in QQ, if I want to implement the p-adic heights over number fields.
You can do exp of an element of QQ just fine.
comment:9 in reply to: ↑ 8 Changed 12 years ago by
Replying to robertwb:
You can do exp of an element of QQ just fine.
Sure, that is not what I meant. I will put all your code of nonarchimedian_local_height
into a helper function that returns r and Nv and then the main function will only take log. So that in the p-adic case I can take p-adic logs of the rational Nv.
But don't worry I will do that when I will implement p-adic height. It was only to remind myself that I should do that.
comment:10 Changed 12 years ago by
The code looks ok to me (I spotted one mis-spelling of Silverman!), and I checked that the results agree with magma, but I'll leave the formal review to Chris who has started and wants to add to the documentation.
comment:11 follow-up: ↓ 12 Changed 12 years ago by
I have put up a rebased patch with a few minor changes. But I have not included yet the documentation on the normalization. You write "THE BSD formula", but there is not a unique standard way of stating the conjecture, I fear. Also the question is whether or not you divide the height pairing by two or not. Could you clarify this ?
I deleted the assumption that E was defined over Q. I don't think you will need that. Maybe it is needed that the model is integral, but I do not see where you would require the curve to be defined over Q. Please correct me if I am wrong.
By the way the diff of our two patches comes mainly from converting tabs to spaces.
comment:12 in reply to: ↑ 11 Changed 12 years ago by
Replying to wuthrich:
I have put up a rebased patch with a few minor changes. But I have not included yet the documentation on the normalization. You write "THE BSD formula", but there is not a unique standard way of stating the conjecture, I fear. Also the question is whether or not you divide the height pairing by two or not. Could you clarify this ?
OK, I have expanded the normalization section, borrowing heavily from the explanation found in John Cremona's book.
I deleted the assumption that E was defined over Q. I don't think you will need that. Maybe it is needed that the model is integral, but I do not see where you would require the curve to be defined over Q. Please correct me if I am wrong.
Yes, you are correct. (There are examples to this effect.)
By the way the diff of our two patches comes mainly from converting tabs to spaces.
They were not of my doing, but thanks for expunging them.
comment:13 Changed 12 years ago by
- Reviewers set to Chris Wuthrich, John Cremona
- Status changed from needs_review to positive_review
New patch applies fine to 4.3.4.alpha1, and all tests in sage/schemes/elliptic_curves pass (tested both 32 and 64 bit). Good!
comment:14 Changed 12 years ago by
- Status changed from positive_review to needs_work
This doesn't apply cleanly to Sage 4.4.alpha0; apparently it conflicts with a patch merged into that. Please rebase it, and we'll try hard to get it into 4.4.alpha1.
Changed 12 years ago by
comment:15 Changed 12 years ago by
- Status changed from needs_work to needs_review
OK, rebased. It looks like it was just deleting some tabs that someone else also deleted, which was easy to fix.
comment:16 Changed 12 years ago by
- Status changed from needs_review to positive_review
I'm remarking this as positive review as there was no content change in the rebase.
comment:17 Changed 12 years ago by
- Merged in set to sage-4.4.alpha1
- Resolution set to fixed
- Status changed from positive_review to closed
Merged "8496-rebased-4.4.alpha0.patch" into 4.4.alpha1.
Very good. I was hoping that someone would implemented it soon ! Thanks a lot.
I will have a look at it. (And add more documentation to it : You normalised the height to be independent of the base field, which is one of the two possible normalisations, and it should be documented)