Opened 12 years ago

Closed 12 years ago

# heegner_index_bound may be incorrect for curves with rational torsion

Reported by: Owned by: rlm cremona major sage-4.4.4 elliptic curves was sage-4.4.4.alpha0 Robert Miller John Cremona N/A

### Description

Result of a recent conversation with William Stein.

### 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).

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