Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tamagawa_numbers method for elliptic curves over number fields #9387

Closed
RalphieBoy mannequin opened this issue Jun 29, 2010 · 11 comments
Closed

Add tamagawa_numbers method for elliptic curves over number fields #9387

RalphieBoy mannequin opened this issue Jun 29, 2010 · 11 comments

Comments

@RalphieBoy
Copy link
Mannequin

RalphieBoy mannequin commented Jun 29, 2010

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.

Component: elliptic curves

Keywords: tamagawa number

Author: Justin Walker

Reviewer: David Loeffler

Merged: sage-4.5.2.alpha0

Issue created by migration from https://trac.sagemath.org/ticket/9387

@RalphieBoy RalphieBoy mannequin added this to the sage-4.5.2 milestone Jun 29, 2010
@RalphieBoy RalphieBoy mannequin assigned JohnCremona Jun 29, 2010
@RalphieBoy
Copy link
Mannequin Author

RalphieBoy mannequin commented Jun 29, 2010

comment:1

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

@RalphieBoy RalphieBoy mannequin added the s: needs review label Jun 29, 2010
@RalphieBoy
Copy link
Mannequin Author

RalphieBoy mannequin commented Jun 29, 2010

comment:2

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

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Jun 30, 2010

comment:3

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.

@loefflerd loefflerd mannequin changed the title Add method for elliptic curves over number fields Add tamagawa_numbers method for elliptic curves over number fields Jun 30, 2010
@JohnCremona
Copy link
Member

comment:4

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!

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Jul 2, 2010

comment:6
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.

@loefflerd loefflerd mannequin added s: needs work and removed s: needs review labels Jul 2, 2010
@RalphieBoy
Copy link
Mannequin Author

RalphieBoy mannequin commented Jul 2, 2010

comment:7

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

@RalphieBoy
Copy link
Mannequin Author

RalphieBoy mannequin commented Jul 2, 2010

Attachment: 9387.patch.gz

New version of patch following DavidL's suggestion

@RalphieBoy
Copy link
Mannequin Author

RalphieBoy mannequin commented Jul 2, 2010

comment:8

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.

@RalphieBoy RalphieBoy mannequin added s: needs review and removed s: needs work labels Jul 2, 2010
@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Jul 3, 2010

Reviewer: David Loeffler

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Jul 3, 2010

comment:9

Looks good to me.

@qed777
Copy link
Mannequin

qed777 mannequin commented Jul 20, 2010

Merged: sage-4.5.2.alpha0

@qed777 qed777 mannequin removed the s: positive review label Jul 20, 2010
@qed777 qed777 mannequin closed this as completed Jul 20, 2010
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant