Opened 7 years ago

Last modified 3 years ago

#13436 needs_work enhancement

Elliptic Curve Enumeration by height

Reported by: spice Owned by: spice
Priority: minor Milestone: sage-7.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) Commit: 8721f1d979c937df37215f19d74f252995f39c88
Dependencies: Stopgaps:

Description (last modified by was)

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, 2-Selmer 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.

https://github.com/haikona/CBH

Attachments (2)

trac_13436_curve_enumerator.patch (84.4 KB) - added by spice 7 years ago.
trac_13436_curve_enumerator-addon1.patch (95.3 KB) - added by chapoton 7 years ago.

Download all attachments as: .zip

Change History (29)

Changed 7 years ago by spice

comment:1 Changed 7 years ago by spice

  • Status changed from new to needs_review

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.

comment:2 Changed 7 years ago by was

  • Description modified (diff)

comment:3 Changed 7 years ago by was

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 7 years ago by was

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):
Last edited 7 years ago by was (previous) (diff)

comment:5 Changed 7 years ago by was

  • Here: over Q up I would put `\\QQ` or at QQ.
  • 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/n-torsion subgroup. If True, the
             n-torsion rank is computed (i.e. 0, 1 or 2). If set to False, the
             size of the n-torsion 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?
Last edited 7 years ago by was (previous) (diff)

comment:6 Changed 7 years ago by ohanar

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 7 years ago by ohanar

  • Status changed from needs_review to needs_work

comment:8 Changed 7 years ago by chapoton

I have uploaded a patch that cleans up the documentation.

Still needs works, I think

comment:9 Changed 7 years ago by chapoton

could please someone with magma add the missing doctest for "three_selmer" ?

comment:10 Changed 7 years ago by cremona

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                                                                                                        
[]
Last edited 7 years ago by cremona (previous) (diff)

Changed 7 years ago by chapoton

comment:11 Changed 7 years ago by chapoton

thanks for the help !

comment:12 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:13 Changed 6 years ago by chapoton

  • Branch set to u/chapoton/13436
  • Commit set to a3af039fac1fc3bce0b1e89049e059a7cc737658

New commits:

a3af039trac 13436 : doc corrections
e405730Add functionality to Sage that allows one to enumerate elliptic curves over Q by height for a range of Weierstrass families of curve.

comment:14 Changed 6 years ago by cremona

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 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:16 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:17 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:18 Changed 5 years ago by git

  • Commit changed from a3af039fac1fc3bce0b1e89049e059a7cc737658 to 0f75b4f7d6b1b17462d2b4ff12d6195d440f79ca

Branch pushed to git repo; I updated commit sha1. New commits:

ba097ecMerge branch 'u/chapoton/13436' of ssh://trac.sagemath.org:22/sage into 13436
0f75b4ftrac #13436 minor changes after some of the reviewer's comments

comment:19 Changed 5 years ago by git

  • Commit changed from 0f75b4f7d6b1b17462d2b4ff12d6195d440f79ca to 512eb17fe656ebeb68902e44b560cea3bc175143

Branch pushed to git repo; I updated commit sha1. New commits:

b4b547bMerge branch 'u/chapoton/13436' into 6.8.b2
512eb17trac #13436 fixing one error

comment:20 Changed 5 years ago by chapoton

  • Milestone changed from sage-6.4 to sage-6.8

There is still one failing doctest, that I do not understand.

comment:21 Changed 4 years ago by chapoton

  • Milestone changed from sage-6.8 to sage-6.10

comment:22 Changed 4 years ago by chapoton

  • Milestone changed from sage-6.10 to sage-7.0

comment:23 Changed 4 years ago by chapoton

  • Milestone changed from sage-7.0 to sage-7.1

comment:24 Changed 4 years ago by git

  • Commit changed from 512eb17fe656ebeb68902e44b560cea3bc175143 to 3fc917ce8f002c6c9d8fa4909d45b2b613aa1cc1

Branch pushed to git repo; I updated commit sha1. New commits:

d4fe9b6Merge branch 'u/chapoton/13436' into 7.1.b2
73502c0trac #13436 updating cartesian_products
3fc917ctrac #13436 pep8

comment:25 Changed 4 years ago by git

  • Commit changed from 3fc917ce8f002c6c9d8fa4909d45b2b613aa1cc1 to 3785184dd352d0ca728f0fcb78e7e025da0d5939

Branch pushed to git repo; I updated commit sha1. New commits:

850d7a3Merge branch 'u/chapoton/13436' into 7.2.b4
3785184trac #13426 fixing one import

comment:26 Changed 4 years ago by git

  • Commit changed from 3785184dd352d0ca728f0fcb78e7e025da0d5939 to 23fa861f318c16e0f8076c8580aa3d8ad719016c

Branch pushed to git repo; I updated commit sha1. New commits:

23fa861Merge branch 'u/chapoton/13436' in 7.3.rc0

comment:27 Changed 3 years ago by git

  • Commit changed from 23fa861f318c16e0f8076c8580aa3d8ad719016c to 8721f1d979c937df37215f19d74f252995f39c88

Branch pushed to git repo; I updated commit sha1. New commits:

04d7e40Merge branch 'u/chapoton/13436' in 7.6.b5
8721f1dtrac 13436 details
Note: See TracTickets for help on using tickets.