Opened 12 years ago

Closed 12 years ago

#9387 closed defect (fixed)

Add tamagawa_numbers method for elliptic curves over number fields

Reported by: Justin Walker Owned by: John Cremona
Priority: major Milestone: sage-4.5.2
Component: elliptic curves Keywords: elliptic curve, tamagawa number
Cc: Merged in: sage-4.5.2.alpha0
Authors: Justin Walker Reviewers: David Loeffler
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

Elliptic curves over the rationals have a method that returns a list of tamagawa numbers for the curve. There is no such method in the case of number fields.

Attachments (1)

9387.patch (2.8 KB) - added by Justin Walker 12 years ago.
New version of patch following DavidL's suggestion

Download all attachments as: .zip

Change History (11)

comment:1 Changed 12 years ago by Justin Walker

Status: newneeds_review

Added patch with the method tamagawa_numbers(), essentially a duplication of the code for the rational case.

comment:2 Changed 12 years ago by Justin Walker

Updated the patch with a corrected doctest (run and passed).

comment:3 Changed 12 years ago by David Loeffler

Summary: Add method for elliptic curves over number fieldsAdd tamagawa_numbers method for elliptic curves over number fields

Just a suggestion: why duplicate the code? Since EllipticCurve_rational_field inherits from EllipticCurve_number_field, you could just *move* the code. Then we don't have the overhead of maintaining two routines doing exactly the same, and the likelihood of bugs is halved. Compare John's regulator patch at #9372.

comment:4 Changed 12 years ago by John Cremona

Status: needs_reviewneeds_work

That's a good idea. Justin, all that's needed is (a) delete the version in ell_rational_field, (b) make sure that the code in ell_number_field works over Q (say by moving the old doctest into the new function -- it should have examples over Q and over another field.

There might be other functions like this. If you notice any, make a ticket!

comment:5 Changed 12 years ago by Justin Walker

Status: needs_workneeds_review

comment:6 Changed 12 years ago by David Loeffler

Status: needs_reviewneeds_work
sage -t  ell_number_field.py
**********************************************************************
File "/storage/masiao/sage-4.5.alpha1/devel/sage-reviewing/sage/schemes/elliptic_curves/ell_number_field.py", line 1049:
    sage: K=NumberField(x^2+3)
Exception raised:
    Traceback (most recent call last):
      File "/storage/masiao/sage-4.5.alpha1/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/storage/masiao/sage-4.5.alpha1/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/storage/masiao/sage-4.5.alpha1/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_22[5]>", line 1, in <module>
        K=NumberField(x**Integer(2)+Integer(3))###line 1049:
    sage: K=NumberField(x^2+3)
      File "/storage/masiao/sage-4.5.alpha1/local/lib/python/site-packages/sage/rings/number_field/number_field.py", line 431, in NumberField
        raise TypeError, "You must specify the name of the generator."
    TypeError: You must specify the name of the generator.
**********************************************************************                           

You should also probably delete, rather than comment out, the code in ell_rational_field.

comment:7 Changed 12 years ago by Justin Walker

OK, that's weird. Turns out I popped when I should have pushed, so I was testing unmodified code. I'll be back.

Changed 12 years ago by Justin Walker

Attachment: 9387.patch added

New version of patch following DavidL's suggestion

comment:8 Changed 12 years ago by Justin Walker

Status: needs_workneeds_review

New patch, replacing previous one. This time, with some luck, I verified the patch against both 4.4.4 and 4.5.a1.

Comments, brickbats, scotch all welcome.

comment:9 Changed 12 years ago by David Loeffler

Reviewers: David Loeffler
Status: needs_reviewpositive_review

Looks good to me.

comment:10 Changed 12 years ago by Mitesh Patel

Merged in: sage-4.5.2.alpha0
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.