Opened 9 years ago

Closed 9 years ago

#11347 closed defect (fixed)

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

Reported by: was Owned by: cremona
Priority: critical Milestone: sage-4.7.1
Component: elliptic curves Keywords:
Cc: Merged in: sage-4.7.1.alpha2
Authors: William Stein Reviewers: John Cremona
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by was)

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!

Attachments (1)

trac_11347.patch (1.6 KB) - added by was 9 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 9 years ago by was

  • Description modified (diff)

comment:2 Changed 9 years ago by was

  • Description modified (diff)

Changed 9 years ago by was

comment:3 Changed 9 years ago by was

  • Status changed from new to needs_review

comment:4 Changed 9 years ago by cremona

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

Testing now.

comment:5 Changed 9 years ago by cremona

  • Authors set to William Stein
  • Reviewers set to John Cremona
  • Status changed from needs_review to positive_review

Applies fine to 4.7.rc1, tests pass.

comment:6 Changed 9 years ago by cremona

See the comment (#4) at #11346.

comment:7 Changed 9 years ago by jdemeyer

  • Merged in set to sage-4.7.1.alpha2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.