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 | Owned by: | 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: |
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)
Change History (11)
comment:1 Changed 12 years ago by
- Status changed from new to needs_review
comment:2 Changed 12 years ago by
Updated the patch with a corrected doctest (run and passed).
comment:3 Changed 12 years ago by
- Summary changed from Add method for elliptic curves over number fields to Add 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
- Status changed from needs_review to needs_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
- Status changed from needs_work to needs_review
comment:6 Changed 12 years ago by
- Status changed from needs_review to needs_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
OK, that's weird. Turns out I popped when I should have pushed, so I was testing unmodified code. I'll be back.
comment:8 Changed 12 years ago by
- Status changed from needs_work to needs_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
- Reviewers set to David Loeffler
- Status changed from needs_review to positive_review
Looks good to me.
comment:10 Changed 12 years ago by
- Merged in set to sage-4.5.2.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
Added patch with the method tamagawa_numbers(), essentially a duplication of the code for the rational case.