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 )
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)
Change History (8)
comment:1 Changed 9 years ago by
- Description modified (diff)
comment:2 Changed 9 years ago by
- Description modified (diff)
Changed 9 years ago by
comment:3 Changed 9 years ago by
- Status changed from new to needs_review
comment:4 Changed 9 years ago by
comment:5 Changed 9 years ago by
- 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
comment:7 Changed 9 years ago by
- Merged in set to sage-4.7.1.alpha2
- Resolution set to fixed
- Status changed from positive_review to closed
Patch looks good to me -- the typo could well have been mine...
Testing now.