Opened 8 years ago

Closed 8 years ago

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

Reported by: Owned by: wuthrich cremona major sage-5.13 elliptic curves local_data sage-5.13.beta0 Chris Wuthrich Frédéric Chapoton, John Cremona N/A merge with #15243

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.

### comment:1 follow-up: ↓ 2 Changed 8 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):
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):
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):
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):
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 8 years ago by cremona (previous) (diff)

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

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 8 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 8 years ago by wuthrich

• Description modified (diff)

### comment:5 Changed 8 years ago by wuthrich

• Description modified (diff)

### comment:6 Changed 8 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 8 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 8 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 8 years ago by wuthrich

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

### comment:9 follow-up: ↓ 10 Changed 8 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 8 years ago by wuthrich

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 8 years ago by jdemeyer

• Milestone changed from sage-5.11 to sage-5.12

### comment:12 follow-up: ↓ 13 Changed 8 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 8 years ago by cremona

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 8 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 8 years ago by jdemeyer

• Status changed from positive_review to needs_info

Which patch(es) should be applied?

### comment:16 Changed 8 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 8 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 8 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 8 years ago by jdemeyer

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

See #15243.

### comment:21 Changed 8 years ago by jdemeyer

• Dependencies set to #15243

### comment:22 Changed 8 years ago by jdemeyer

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

### comment:23 Changed 8 years ago by jdemeyer

• Milestone changed from sage-5.12 to sage-5.13

### comment:24 Changed 8 years ago by jdemeyer

• Dependencies changed from #15243 to merge with #15243

### comment:25 Changed 8 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 8 years ago by jdemeyer

• Milestone changed from sage-5.13 to sage-pending

### comment:27 Changed 8 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.