Opened 7 years ago

Closed 6 years ago

#14476 closed defect (fixed)

non-integral models can cause a bug in local_data for elliptic curves over number fields

Reported by: wuthrich Owned by: cremona
Priority: major Milestone: sage-5.13
Component: elliptic curves Keywords: local_data
Cc: Merged in: sage-5.13.beta0
Authors: Chris Wuthrich Reviewers: Frédéric Chapoton, John Cremona
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: merge with #15243 Stopgaps:

Description (last modified by jdemeyer)

This example was spotted by Dino Lorenzini:

sage: t = QQ['t'].0
sage: K.<g> = NumberField(t^4 - t^3-3*t^2 - t +1)
sage: x=-3/5*g^3 + 2/5*g^2 + 3/5*g + 4/5
sage: y=1/15*g^3 - 3/5*g^2 + 3/5*g + 17/15
sage: r = (x^2*y - x*y + y - 1) / (x^2*y - x)
sage: s = (x*y - y + 1) / (x*y)
sage: c = s*(r - 1)
sage: b = c*r
sage: E = EllipticCurve(K,[1-c,-b,-b,0,0])
sage: E.local_data()

which causes a TypeError in _reduce_local. Tickets that may be linked to this are #11630 and #9410.

Added later: Moreover I can add another bug that he showed me

sage: R.<t> = QQ[]
sage: K.<g> = NumberField(t^4 - t^3 - 3*t^2 - t + 1)
sage: E = EllipticCurve([ -43/625*g^3 + 14/625*g^2 - 4/625*g + 706/625, -4862/78125*g^3 - 4074/78125*g^2 - 711/78125*g + 10304/78125,  -4862/78125*g^3 - 4074/78125*g^2 - 711/78125*g + 10304/78125, 0,0])
sage: E.global_integral_model()

ends in a AssertionError: bug in global_integral_model.

APPLY:

NOTE: 32-bit doctests require #15243 to be applied on top of these patches.

Attachments (2)

trac_14476_bugs_in_local_data.patch (5.1 KB) - added by wuthrich 7 years ago.
The attached patch was exported from 5.9 after having applied the patched for #12509 #13953 #11630 in that order. I
trac_14476_trac_links.patch (5.0 KB) - added by chapoton 7 years ago.

Download all attachments as: .zip

Change History (29)

comment:1 follow-up: Changed 7 years ago by cremona

The reason is this:

sage: E.is_global_integral_model()
False

and if one replaces E by a global integral model then all is well:

sage: E1 = E.global_integral_model()                                                                           
sage: E1.local_data()                                                                                          
[Local data at Fractional ideal (g^3 - g^2 - 2*g - 1):
Reduction type: bad split multiplicative
Local minimal model: Elliptic Curve defined by y^2 + (g^3+g)*x*y + g*y = x^3 + (-g^2+1)*x^2 + (126914883*g^3-73346242*g^2-411702677*g-300687331)*x + (-1068031359960*g^3+617234077764*g^2+3464617746565*g+2530385673583) over Number Field in g with defining polynomial t^4 - t^3 - 3*t^2 - t + 1
Minimal discriminant valuation: 11
Conductor exponent: 1
Kodaira Symbol: I11
Tamagawa Number: 11,
 Local data at Fractional ideal (-g^2 + g + 1):
Reduction type: bad split multiplicative
Local minimal model: Elliptic Curve defined by y^2 + (g^3+g)*x*y + g*y = x^3 + (-g^2+1)*x^2 + (126914883*g^3-73346242*g^2-411702677*g-300687331)*x + (-1068031359960*g^3+617234077764*g^2+3464617746565*g+2530385673583) over Number Field in g with defining polynomial t^4 - t^3 - 3*t^2 - t + 1
Minimal discriminant valuation: 11
Conductor exponent: 1
Kodaira Symbol: I11
Tamagawa Number: 11,
 Local data at Fractional ideal (g^2 - g - 2):
Reduction type: bad split multiplicative
Local minimal model: Elliptic Curve defined by y^2 + (g^3+g)*x*y + g*y = x^3 + (-g^2+1)*x^2 + (126914883*g^3-73346242*g^2-411702677*g-300687331)*x + (-1068031359960*g^3+617234077764*g^2+3464617746565*g+2530385673583) over Number Field in g with defining polynomial t^4 - t^3 - 3*t^2 - t + 1
Minimal discriminant valuation: 11
Conductor exponent: 1
Kodaira Symbol: I11
Tamagawa Number: 11,
 Local data at Fractional ideal (3*g^3 - 12*g - 4):
Reduction type: bad non-split multiplicative
Local minimal model: Elliptic Curve defined by y^2 + (g^3+g)*x*y + g*y = x^3 + (-g^2+1)*x^2 + (126914883*g^3-73346242*g^2-411702677*g-300687331)*x + (-1068031359960*g^3+617234077764*g^2+3464617746565*g+2530385673583) over Number Field in g with defining polynomial t^4 - t^3 - 3*t^2 - t + 1
Minimal discriminant valuation: 1
Conductor exponent: 1
Kodaira Symbol: I1
Tamagawa Number: 1]

Of course it would be possible to have local_data() do this. At the very least, local data should give a more polite message.

Last edited 7 years ago by cremona (previous) (diff)

comment:2 in reply to: ↑ 1 Changed 7 years ago by wuthrich

Replying to cremona:

Of course it would be possible to have local_data() do this. At the very least, local data should give a more polite message.

Not only that. The documentation says that non-integral model are allowed. And I firmly believe they should be. I am not in favour of making the model globally integral beforehand, again because it should stay a local algorithm.

comment:3 Changed 7 years ago by wuthrich

Just so that I don't forget what I found out so far :

The issue, as silly as it sounds, comes up from __repr__ this class. This does not display _Emin the minimal equation that Tate's algorithm found, instead it wants to compute minimal_model. This does nothing but calling _reduced_model in ell_number_fields.py. And that is global again. So in _reduce_model it checks if the coefficients are integers, while we only need that they are local integers here.

The quick solution I see is to add to the local class a function to reduce the model locally. But I will have to check what that is going to break.

comment:4 Changed 7 years ago by wuthrich

  • Description modified (diff)

comment:5 Changed 7 years ago by wuthrich

  • Description modified (diff)

comment:6 Changed 7 years ago by wuthrich

OK, I solved the second problem in global_integral_model. The algorithm did not check that e was negative - for indeed a uniformiser for one place could be one for another and then the non-integrality at the later was already done. Rather than just adding if e<0: I decided to rewrite it to make it do a minimal number of changes. Patch to come.

comment:7 Changed 7 years ago by wuthrich

I changed my mind about the first problem. There is no meaning to a local version of reduced equation. So in order not to break anything, I propose to reduce the local minimal equation only if it is globally integral coefficients.

The attached patch was exported from 5.9 after having applied the patched for #12509 #13953 #11630 in that order. It solves the two above issues.

Changed 7 years ago by wuthrich

The attached patch was exported from 5.9 after having applied the patched for #12509 #13953 #11630 in that order. I

comment:8 Changed 7 years ago by wuthrich

  • Authors set to Chris Wuthrich
  • Status changed from new to needs_review

Changed 7 years ago by chapoton

comment:9 follow-up: Changed 7 years ago by chapoton

I have added a review patch that adds all missing links to trac (using the trac role :trac:)

comment:10 in reply to: ↑ 9 Changed 7 years ago by wuthrich

Replying to chapoton:

I have added a review patch that adds all missing links to trac (using the trac role :trac:)

Thanks. Can you afterwards give a positive review ?

comment:11 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:12 follow-up: Changed 6 years ago by chapoton

I think this ticket only needs a "mathematical green light" to get a positive review. As I am not an elliptic guy, I would prefer if some expert would enter here and say let's go.

comment:13 in reply to: ↑ 12 Changed 6 years ago by cremona

Replying to chapoton:

I think this ticket only needs a "mathematical green light" to get a positive review. As I am not an elliptic guy, I would prefer if some expert would enter here and say let's go.

I am happy to give this the required green light -- have not tested, but I understand the problem and the solution looks good. So as long as the patches apply to 5.12.beta4 and tests pass, I am happy with it getting positive review.

comment:14 Changed 6 years ago by chapoton

  • Reviewers set to Frédéric Chapoton, John Cremona
  • Status changed from needs_review to positive_review

ok, then positive review.

comment:15 Changed 6 years ago by jdemeyer

  • Status changed from positive_review to needs_info

Which patch(es) should be applied?

comment:16 Changed 6 years ago by chapoton

  • Description modified (diff)
  • Status changed from needs_info to positive_review

both patches should be applied, as listed now at bottom of the description

comment:17 Changed 6 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Does the algorithm depend on choices? On arando (32-bit Ubuntu 13.04):

sage -t --long devel/sage/sage/schemes/elliptic_curves/ell_number_field.py
**********************************************************************
File "devel/sage/sage/schemes/elliptic_curves/ell_number_field.py", line 605, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.global_integral_model
Failed example:
    E.global_integral_model()
Expected:
    Elliptic Curve defined by y^2 + (-18*g^3+29*g^2+63*g+7)*x*y + (-704472*g^3-958584*g^2-166242*g+298101)*y = x^3 + (-2859*g^3-3978*g^2-669*g+1332)*x^2 over Number Field in g with defining polynomial t^4 - t^3 - 3*t^2 - t + 1
Got:
    Elliptic Curve defined by y^2 + (-5*g^3+g^2+9*g+5)*x*y + (57305*g^3+78489*g^2+14025*g-24161)*y = x^3 + (-541*g^3-738*g^2-126*g+232)*x^2 over Number Field in g with defining polynomial t^4 - t^3 - 3*t^2 - t + 1
**********************************************************************

comment:18 Changed 6 years ago by jdemeyer

Indeed, the uniformizer is not unique:

On 64-bit:

sage: K.<t> = NumberField(x^4 - x^3 - 3*x^2 - x + 1)
sage: [K.uniformizer(P) for P,e in factor(K.ideal(5))]
[t^2 - t + 1, t^3 - t^2 - 4*t - 1, -t^3 + 2*t^2 + 3*t - 1]

On 32-bit:

sage: K.<t> = NumberField(x^4 - x^3 - 3*x^2 - x + 1)
sage: [K.uniformizer(P) for P,e in factor(K.ideal(5))]
[t^2 - t + 1, t^2 - t - 1, -t^3 + 3*t + 2]

comment:19 Changed 6 years ago by jdemeyer

I think the best solution is to "fix" the uniformizer function, I'm thinking about a patch...

comment:20 Changed 6 years ago by jdemeyer

See #15243.

comment:21 Changed 6 years ago by jdemeyer

  • Dependencies set to #15243

comment:22 Changed 6 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from needs_work to positive_review

comment:23 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.12 to sage-5.13

comment:24 Changed 6 years ago by jdemeyer

  • Dependencies changed from #15243 to merge with #15243

comment:25 Changed 6 years ago by cremona

Thanks, I think that is the best option. I am looking at your patch on #15243 now. I wonder if there is any chance of persuading Karim et al to make all of pari independent of 32/64 bit. Whatever reasons there were are surely no longer valid!

comment:26 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.13 to sage-pending

comment:27 Changed 6 years ago by jdemeyer

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