Opened 9 years ago
Last modified 5 years ago
#13436 needs_work enhancement
Elliptic Curve Enumeration by height
Reported by:  spice  Owned by:  spice 

Priority:  minor  Milestone:  sage7.1 
Component:  elliptic curves  Keywords:  Elliptic curves, enumeration 
Cc:  Merged in:  
Authors:  Simon Spicer  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  u/chapoton/13436 (Commits, GitHub, GitLab)  Commit:  8721f1d979c937df37215f19d74f252995f39c88 
Dependencies:  Stopgaps: 
Description (last modified by )
Add functionality to Sage that allows one to enumerate elliptic curves over Q by height for a range of Weierstrass families of curve.
This code has grown from work with Wei Ho, who desired a way to enumerate curves over Q by height (where the height of a curve is a function of the coefficients in its Weierstrass model) so that the average value of some datum (e.g.rank, 2Selmer size) could be computed for curves up to a given height. This code introduces a class, CurveEnumerator?(), that computes lists of curves ordered by height for this purpose.
Attachments (2)
Change History (29)
Changed 9 years ago by
comment:1 Changed 9 years ago by
 Status changed from new to needs_review
comment:2 Changed 9 years ago by
 Description modified (diff)
comment:3 Changed 9 years ago by
For the three_selmer function, you need a doctest.
sage: magma('2+2') # optional  magma
Then
sage t only_optional=magma curve_enumerator.py
to test.
comment:4 Changed 9 years ago by
Fix some docstring formatitng, e.g., the funny indent:
def heights(self, lowerbound, upperbound): """ Return a list of permissable curve heights in the specified range (bounds inclusive), and for each height the equation coefficients that produce curves of that height.
This should output "100%":
...elliptic_curves$ sage coverage curve_enumerator.py  curve_enumerator.py ERROR: Please add a `TestSuite(s).run()` doctest. SCORE curve_enumerator.py: 87% (35 of 40) Missing documentation: * __init__(self): * __repr__(self): * _coeffs_to_a_invariants(self, c): Missing doctests: * __init__(self): * three_selmer(self, curves, rank=True, reduced=False, output_filename=None, problems_filename=None, \ proof=True, return_data=True, print_timing=True):
comment:5 Changed 9 years ago by
 Here:
over Q up
I would put`\\QQ`
or atQQ
.
 Either: Or:  maybe instead a sublist of two things?
 weird formatting:
sage: C = EllipticCurveEnumerator(family="short_weierstrass") sage: C.n_torsion? * "rank"  (Default True): Compute the rank versus size of the curve's torsion/ntorsion subgroup. If True, the ntorsion rank is computed (i.e. 0, 1 or 2). If set to False, the size of the ntorsion subgroup is computed instead (i.e. n^rank) * "output_filename"  (Default None): If not None, the string name of the file to which the output will be saved.
 Restructure code so that this is natural:
v = EllipticCurveEnumerator(family="short_weierstrass", height=1000).ranks()
Will need caching:
sage: from sage.misc.cachefunc import cached_method sage: cached_method?
comment:6 Changed 9 years ago by
There are three AssertionErrors
that should be ValueErrors
.
also two_selmer
and three_selmer
have an extra space for the OUTPUT portion of the docstring.
comment:7 Changed 9 years ago by
 Status changed from needs_review to needs_work
comment:8 Changed 8 years ago by
I have uploaded a patch that cleans up the documentation.
Still needs works, I think
comment:9 Changed 8 years ago by
could please someone with magma add the missing doctest for "three_selmer" ?
comment:10 Changed 8 years ago by
I applied the patches to 5.10 and ran the doctest and report the result here.
sage: C = EllipticCurveEnumerator(family="short_weierstrass") sage: L = C.coefficients_over_height_range(4, 4) sage: R, P = C.three_selmer(L, rank=True,return_data=True,print_timing=False) sage: R [(4, [0, 0, 0, 1, 2], 1), (4, [0, 0, 0, 1, 2], 0), (4, [0, 0, 0, 0, 2], 1), (4, [0, 0, 0, 0, 2], 1), (4, [0, 0, 0, 1, 2], 0), (4, [0, 0, 0, 1, 2], 0)] sage: P []
Changed 8 years ago by
comment:11 Changed 8 years ago by
thanks for the help !
comment:12 Changed 8 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:13 Changed 8 years ago by
 Branch set to u/chapoton/13436
 Commit set to a3af039fac1fc3bce0b1e89049e059a7cc737658
comment:14 Changed 8 years ago by
Thanks for converting this to git and fixing the docstring formatting (which I will trust has been done properly)!
Authors: can you remove the place(s) where there are comments "this should be checked" or "todo"? And in several similar places you can delete the comment that some integers must be Sage integers or ... some rather frivolous consequence will occur?
I see that you have carefully allowed for the unavoidable fact that there will be "problem curves" and have an option output filename for these. But I don't think that any of the doctests illustrate this  please add one or more doctests which do. Meanwhile I am trying the functions out.
comment:15 Changed 8 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:16 Changed 7 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:17 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:18 Changed 7 years ago by
 Commit changed from a3af039fac1fc3bce0b1e89049e059a7cc737658 to 0f75b4f7d6b1b17462d2b4ff12d6195d440f79ca
comment:19 Changed 6 years ago by
 Commit changed from 0f75b4f7d6b1b17462d2b4ff12d6195d440f79ca to 512eb17fe656ebeb68902e44b560cea3bc175143
comment:20 Changed 6 years ago by
 Milestone changed from sage6.4 to sage6.8
There is still one failing doctest, that I do not understand.
comment:21 Changed 6 years ago by
 Milestone changed from sage6.8 to sage6.10
comment:22 Changed 6 years ago by
 Milestone changed from sage6.10 to sage7.0
comment:23 Changed 6 years ago by
 Milestone changed from sage7.0 to sage7.1
comment:24 Changed 6 years ago by
 Commit changed from 512eb17fe656ebeb68902e44b560cea3bc175143 to 3fc917ce8f002c6c9d8fa4909d45b2b613aa1cc1
comment:25 Changed 5 years ago by
 Commit changed from 3fc917ce8f002c6c9d8fa4909d45b2b613aa1cc1 to 3785184dd352d0ca728f0fcb78e7e025da0d5939
comment:26 Changed 5 years ago by
 Commit changed from 3785184dd352d0ca728f0fcb78e7e025da0d5939 to 23fa861f318c16e0f8076c8580aa3d8ad719016c
Branch pushed to git repo; I updated commit sha1. New commits:
23fa861  Merge branch 'u/chapoton/13436' in 7.3.rc0

comment:27 Changed 5 years ago by
 Commit changed from 23fa861f318c16e0f8076c8580aa3d8ad719016c to 8721f1d979c937df37215f19d74f252995f39c88
Version 1.0 patch posted. All new code (except for an added import statement in sage.schemes.elliptic_curves.all) is in a new file, curve_enumerator.py, in the sage.schemes.elliptic_curves directory. It's thoroughly documented and doctested, so hopefully its logic will straightforward enough to follow.