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

global_minimal_model function is sometimes wrong over number fields, when input model isn't integral. #11347

Closed
williamstein opened this issue May 17, 2011 · 9 comments

Comments

@williamstein
Copy link
Contributor

The discriminant and conductor of a global minimal model must be divisible by the same primes. However the following code (extracted from examples computed by Joanna Gaski), illustrates the Sage global_minimal_model function producing a model that can't possibly be a global minimal model (since the conductor and discriminant are divisible by different primes).

sage: K.<g> = NumberField(x^2 - x - 1)
sage: E = EllipticCurve(K,[0,0,0,-1/48,161/864]).global_minimal_model(); E
Elliptic Curve defined by y^2 = x^3 + (-1)*x^2 + 12 over Number Field in g with defining polynomial x^2 - x - 1
sage: E.conductor().factor()
(Fractional ideal (3)) * (Fractional ideal (-2*g + 1))
sage: E.discriminant().factor()
(-1) * 2^12 * 3 * (-2*g + 1)^2

Again, the bug is that the global_minimal_model function is assuming that its input is integral, and the fix is easy, probably.

sage: E = EllipticCurve(K,[0,0,0,-1/48,161/864]).integral_model().global_minimal_model(); E
Elliptic Curve defined by y^2 + x*y + y = x^3 + x^2 over Number Field in g with defining polynomial x^2 - x - 1
sage: E.conductor().factor()
(Fractional ideal (3)) * (Fractional ideal (-2*g + 1))
sage: E.discriminant().factor()
(-1) * 3 * (-2*g + 1)^2

Yes, inspecting the source code shows a typo related to this, i.e., somebody defines E to be a global integral model, then forgets to actually use E!

Component: elliptic curves

Author: William Stein

Reviewer: John Cremona

Merged: sage-4.7.1.alpha2

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

@williamstein

This comment has been minimized.

@williamstein

This comment has been minimized.

@williamstein
Copy link
Contributor Author

comment:3

Attachment: trac_11347.patch.gz

@JohnCremona
Copy link
Member

comment:4

Patch looks good to me -- the typo could well have been mine...

Testing now.

@JohnCremona
Copy link
Member

Reviewer: John Cremona

@JohnCremona
Copy link
Member

comment:5

Applies fine to 4.7.rc1, tests pass.

@JohnCremona
Copy link
Member

Author: William Stein

@JohnCremona
Copy link
Member

comment:6

See the comment (#4) at #11346.

@jdemeyer
Copy link

Merged: sage-4.7.1.alpha2

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

3 participants