Opened 9 years ago
Closed 9 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: | John 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 )
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)
Change History (29)
comment:2 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
Description: | modified (diff) |
---|
comment:5 Changed 9 years ago by
Description: | modified (diff) |
---|
comment:6 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
Attachment: | trac_14476_bugs_in_local_data.patch added |
---|
comment:8 Changed 9 years ago by
Authors: | → Chris Wuthrich |
---|---|
Status: | new → needs_review |
Changed 9 years ago by
Attachment: | trac_14476_trac_links.patch added |
---|
comment:9 follow-up: 10 Changed 9 years ago by
I have added a review patch that adds all missing links to trac (using the trac role :trac:
)
comment:10 Changed 9 years ago by
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 9 years ago by
Milestone: | sage-5.11 → sage-5.12 |
---|
comment:12 follow-up: 13 Changed 9 years ago by
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 Changed 9 years ago by
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 9 years ago by
Reviewers: | → Frédéric Chapoton, John Cremona |
---|---|
Status: | needs_review → positive_review |
ok, then positive review.
comment:15 Changed 9 years ago by
Status: | positive_review → needs_info |
---|
Which patch(es) should be applied?
comment:16 Changed 9 years ago by
Description: | modified (diff) |
---|---|
Status: | needs_info → positive_review |
both patches should be applied, as listed now at bottom of the description
comment:17 Changed 9 years ago by
Status: | positive_review → 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 9 years ago by
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 9 years ago by
I think the best solution is to "fix" the uniformizer
function, I'm thinking about a patch...
comment:21 Changed 9 years ago by
Dependencies: | → #15243 |
---|
comment:22 Changed 9 years ago by
Description: | modified (diff) |
---|---|
Status: | needs_work → positive_review |
comment:23 Changed 9 years ago by
Milestone: | sage-5.12 → sage-5.13 |
---|
comment:24 Changed 9 years ago by
Dependencies: | #15243 → merge with #15243 |
---|
comment:25 Changed 9 years ago by
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 9 years ago by
Milestone: | sage-5.13 → sage-pending |
---|
comment:27 Changed 9 years ago by
Merged in: | → sage-5.13.beta0 |
---|---|
Milestone: | sage-pending → sage-5.13 |
Resolution: | → fixed |
Status: | positive_review → closed |
The reason is this:
and if one replaces E by a global integral model then all is well:
Of course it would be possible to have local_data() do this. At the very least, local data should give a more polite message.