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: |
Description
Result of a recent conversation with William Stein.
Attachments (1)
Change History (14)
comment:1 Changed 12 years ago by
comment:2 Changed 12 years ago by
- Status changed from new to needs_review
comment:3 Changed 12 years ago by
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
- 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
- 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
- Status changed from needs_work to needs_review
Ping! This ticket still needs a review...
comment:7 Changed 12 years ago by
- 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:
- on line 6365: instead of "an interval that contains the index, or the index over 2" say "... or half the index".
- 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?)
- 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
- 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
- Status changed from needs_review to positive_review
Great -- positive review now.
comment:10 Changed 12 years ago by
- 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
comment:11 Changed 12 years ago by
- Status changed from needs_work to needs_review
comment:13 Changed 12 years ago by
- Merged in set to sage-4.4.4.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
I need to fix this to use
two_torsion_rank
instead oftorsion_order
... tomorrow!