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:

Status badges

Description


Attachments (4)

8496-nf-height.patch (13.6 KB) - added by robertwb 12 years ago.
trac_8496_rebased.patch (15.3 KB) - added by wuthrich 12 years ago.
exported against 4.3.4.alpha1
8496-nf-height-again.patch (17.2 KB) - added by robertwb 12 years ago.
Rebased and updated, apply only this patch.
8496-rebased-4.4.alpha0.patch (15.7 KB) - added by robertwb 12 years ago.

Download all attachments as: .zip

Change History (21)

Changed 12 years ago by robertwb

comment:1 Changed 12 years ago by robertwb

  • Status changed from new to needs_review

comment:2 Changed 12 years ago by wuthrich

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)

comment:3 Changed 12 years ago by cremona

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 wuthrich

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 robertwb

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 was

  • 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: Changed 12 years ago by wuthrich

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: Changed 12 years ago by robertwb

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 wuthrich

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 cremona

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.

Changed 12 years ago by wuthrich

exported against 4.3.4.alpha1

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

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 robertwb

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.

Changed 12 years ago by robertwb

Rebased and updated, apply only this patch.

comment:13 Changed 12 years ago by cremona

  • Authors set to Robert Bradshaw, Chris Wuthrich
  • 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 jhpalmieri

  • 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 robertwb

comment:15 Changed 12 years ago by robertwb

  • 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 robertwb

  • 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 jhpalmieri

  • 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.

Note: See TracTickets for help on using tickets.