Opened 12 years ago

Closed 12 years ago

#8238 closed enhancement (fixed)

heegner_index_bound may be incorrect for curves with rational torsion

Reported by: rlm Owned by: cremona
Priority: major Milestone: sage-4.4.4
Component: elliptic curves Keywords:
Cc: was Merged in: sage-4.4.4.alpha0
Authors: Robert Miller Reviewers: John Cremona
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

Result of a recent conversation with William Stein.

Attachments (1)

trac_8238.patch (4.7 KB) - added by rlm 12 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 12 years ago by rlm

I need to fix this to use two_torsion_rank instead of torsion_order... tomorrow!

comment:2 Changed 12 years ago by rlm

  • Status changed from new to needs_review

comment:3 Changed 12 years ago by wuthrich

I wouldn't mind reviewing this, but I am probably not the right person to do so, since I don't really understand what it changes. Would it be good to have a test example included to illustrate the change.

Also in 4.3.3.alpha0 I get a test error in heegner.py, which wuld make my review useless.Sorry.

comment:4 Changed 12 years ago by was

  • Milestone set to sage-4.4
  • Status changed from needs_review to needs_work

Robert,

I think the point of this patch is to change the function so it is no longer off by factors of 2 by default. Note that the documentation, even after applying your patch, says:

    r"""
    Return an interval that contains the index of the Heegner
    point `y_K` in the group of `K`-rational points modulo torsion
    on this elliptic curve, computed using the Gross-Zagier
    formula and/or a point search, or the index divided by `2`.

    .. note::

       If ``min_p`` is bigger than 2 then the index can be off by
       any prime less than ``min_p``. This function returns the
       index divided by `2` exactly when the rank of `E(K)` is
       greater than 1 and `E(\QQ)_{/tor} \oplus E^D(\QQ)_{/tor}`
       has index `2` in `E(K)_{/tor}`, where the second factor
       undergoes a twist.

If you've really fixed the "factor of 2" issue, as it seems you have, then the documentation should be changed to reflect this. Moreover, this is an enhancement, rather than a bug fix.

comment:5 Changed 12 years ago by rlm

  • Type changed from defect to enhancement

I was able to fix the output for rank 0 and 1 curves, but not in general. Note it says "This function returns the index divided by 2 exactly when the rank of E(K) is greater than 1 and ..."

comment:6 Changed 12 years ago by rlm

  • Status changed from needs_work to needs_review

Ping! This ticket still needs a review...

comment:7 Changed 12 years ago by cremona

  • Reviewers set to John Cremona
  • Status changed from needs_review to needs_work

I applied the patch to 4.4.3 and it worked fine (patch applies cleanly, all long tests in heegner.py pass,

Some minor quibbles:

  1. on line 6365: instead of "an interval that contains the index, or the index over 2" say "... or half the index".
  2. line 6454++: this code is reached whe then rank is 0 (or it appears to be) but then F.gens()[0] would fail. If it is the case that the rank would never be 0 here. I would prefer a comment to that effect and change the test to if F.rank() == 1. (I assume that in this context calling F.rank() and F.gens() is not expensive?)
  3. After setting a=2 in line 6460 the loop should break. (OK, so there will not be that many torsion points o nEK, but still.) Even better would be to first compute EK.torsion_points() and only run the loop at all if it has even length? (But still test if z itself can be divided by 2). I'm sure I once wrote a function which returned a list of points generating torsion modulo 2*torsion....if you did
    sage: for T in EK.torsion_subgroup().gens():
    ....:     if T.order()%2==0:
    

you would have at most 2 points to check.

I'll give this a positive review once these have been fixed.

comment:8 Changed 12 years ago by rlm

  • Status changed from needs_work to needs_review

I think I've addressed each of the above concerns. Thank you for the review!

comment:9 Changed 12 years ago by cremona

  • Status changed from needs_review to positive_review

Great -- positive review now.

comment:10 Changed 12 years ago by cremona

  • Status changed from positive_review to needs_work

very sorry, my mistake -- if there are 2 torsion gens of even order you still needs to also trying adding their sum (i.e. if T/2T has order 4 there should be 4 tests).

Changed 12 years ago by rlm

comment:11 Changed 12 years ago by rlm

  • Status changed from needs_work to needs_review

comment:12 Changed 12 years ago by cremona

  • Status changed from needs_review to positive_review

OK!

comment:13 Changed 12 years ago by mhansen

  • Merged in set to sage-4.4.4.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.