Bug in elliptic curve Galois Representation #19229

JohnCremona · 2015-09-17T16:06:10Z

sage: K.<a> = NumberField(x^2-x+1)
sage: E = EllipticCurve([a+1,1,1,0,0])
sage: C = E.isogeny_class()
...
ValueError: 0 is not prime.

is caused by

sage: from sage.schemes.elliptic_curves.isogeny_class import possible_isogeny_degrees
sage: possible_isogeny_degrees(E)                                                                      
[0]

and in turn by

sage: EG = E.galois_representation()
sage: EG.reducible_primes()
[0]

According to the documentation for the last function it should return [0] if and only if E has CM, which is does not:

sage: E.has_cm()
False
sage: E.j_invariant().is_integral()
False

(CM curves certainly have integral j-invariant, so you don't need to trust the is_cm() method to believe that!)

Component: elliptic curves

Keywords: Galois representations

Author: John Cremona

Branch/Commit: 8680baa

Reviewer: Frédéric Chapoton

Issue created by migration from https://trac.sagemath.org/ticket/19229

The text was updated successfully, but these errors were encountered:

JohnCremona · 2015-09-17T16:16:55Z

comment:1

Tracked down to {{{_semistable_reducible_primes()}} in line 801:

F = NumberField(y**2 - a, 'a')

where a is an integer, but then the code base-changes the base field to this field Q(sqrt(a)), and there is a problem since 'a' is the name of K's generator. This causes a ValueError which is caught in the wrong place in lines 292-4:

        try:
            bad_primes = _semistable_reducible_primes(E)
        except ValueError:
            return [0]

I will fix this by putting in a test for CM much earlier (which did not exist when this code was written). Also when constructing F we can make sure that the name of iots generator is not the same as that of K.

JohnCremona · 2015-09-18T10:28:50Z

Author: John Cremona

JohnCremona · 2015-09-18T10:49:22Z

Commit: df3d3f7

JohnCremona · 2015-09-18T10:49:22Z

New commits:

`df3d3f7`	`19229: e.c. gal.reps. bug fix`

JohnCremona · 2015-09-18T10:49:22Z

Branch: u/cremona/19229

JohnCremona · 2015-09-18T10:52:55Z

comment:4

Fixed as follows: on construction of the GaloisRepresentation object the CM status of the elliptic curve is assessed and cached. This enables some cleaning of the code later where we never have to rely on the raising of exceptions to catch situations which only happen in the CM case. This does not in itself fix the bug, we just get a different error, so the second fix is to make the variable name of a number field constructed on one function not clash.

Appropriate doctests added here and in the docstring for E.isogeny_class().

JohnCremona · 2015-12-08T17:51:28Z

comment:6

Anyone out there?

jdemeyer · 2015-12-08T20:37:09Z

comment:7

What's the point of this "poor man's caching", why not use cached_method?

+        self._has_cm = E.has_cm()
+        self._has_rational_cm = E.has_rational_cm()

There seems to be an indentation problem with

+ # ramified primes

Funny spacing:

+ if E .has_bad_reduction(P):

JohnCremona · 2015-12-09T09:11:48Z

comment:8

Replying to @jdemeyer:

What's the point of this "poor man's caching", why not use cached_method?
+        self._has_cm = E.has_cm()
+        self._has_rational_cm = E.has_rational_cm()

No reason, but that is how it was, and this is a bug fix.

There seems to be an indentation problem with
+ # ramified primes
Funny spacing:
+ if E .has_bad_reduction(P):

I will look at those.

sagetrac-git · 2015-12-09T10:17:31Z

Branch pushed to git repo; I updated commit sha1. New commits:

`83f8108`	`Merge branch 'u/cremona/19229' of trac.sagemath.org:sage into 19229`
`cbd7a97`	`#19229: fixes after review`

sagetrac-git · 2015-12-09T10:17:31Z

Changed commit from df3d3f7 to cbd7a97

JohnCremona · 2015-12-09T10:19:10Z

comment:10

I merged with current beta, fixed the two spacing issues, made 3 methods into cached_methods as suggested. I also tidied up imports in ell_number_field.

Please re-review!

jdemeyer · 2015-12-09T15:03:24Z

comment:11

Did you do something while committing? I see you added files src/sage/schemes/hyperelliptic_curves/hypellfrob.cpp and src/sage/schemes/toric/divisor_class.c

I recommend to fix this using --amend to make sure these added files don't appear in the git history at all.

JohnCremona · 2015-12-09T15:39:54Z

comment:12

That was a mistake. git is showing up hundreds of .c, .cpp, .h files which makes "git status" hard to read and I added some files by mistake but then thought that I had removed them again. What a pain. I'll fix it.

jdemeyer · 2015-12-09T15:47:17Z

comment:13

Replying to @JohnCremona:

git is showing up hundreds of .c, .cpp, .h files which makes "git status" hard to read

This isn't supposed to be like that. Do yourself a favour and remove those files.

sagetrac-git · 2015-12-09T15:49:35Z

Changed commit from cbd7a97 to 62accc0

sagetrac-git · 2015-12-09T15:49:35Z

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

`62accc0`	`19229: fixes after review`

JohnCremona · 2015-12-09T15:51:13Z

comment:15

Replying to @jdemeyer:

Replying to @JohnCremona:

git is showing up hundreds of .c, .cpp, .h files which makes "git status" hard to read

This isn't supposed to be like that. Do yourself a favour and remove those files.

Any idea how they got there? None have anything to do with anything I have done. I presume a simple rm (not git) will suffice?

I fixed the commit...

fchapoton · 2016-02-03T07:44:56Z

comment:25

does no longer apply.

JohnCremona · 2016-02-03T08:35:03Z

comment:26

Dammit, I thought this was merged ages ago. So I'll fix it -- I am using it every day!

sagetrac-git · 2016-02-04T14:40:38Z

Branch pushed to git repo; I updated commit sha1. New commits:

`3ce177a`	`Merge branch 'develop' into 19229`
`de3fdf1`	`minor changes to imports in one file`

sagetrac-git · 2016-02-04T14:40:38Z

Changed commit from e640e42 to de3fdf1

JohnCremona · 2016-02-04T14:41:27Z

comment:28

OK, I merged into 7.0.beta1 and make some import adjustments so it now works.

New commits:

`3ce177a`	`Merge branch 'develop' into 19229`
`de3fdf1`	`minor changes to imports in one file`

fchapoton · 2016-02-04T16:55:01Z

comment:29

two failing doctests !

JohnCremona · 2016-02-04T17:14:14Z

comment:30

I can guess which ones wince I noticed that there are two which sometoimes do work and sometimes do not. Since you do not tell me, I guess that these are the 2 6x6 matrices for the CM isogeny class example with class number 6.

In my defence, I can assure you that on my machine the tests did pass before I uploaded the branch....

fchapoton · 2016-02-04T19:01:44Z

comment:31

To know the failing doctest, you have to click on the patchbot icon (currently yellow with a question mark) at the top right of this page.

Then click on the "shortlog" link of the next-to-last report by "poseidon" patchbot.

This is indeed a 66 matrix of numbers and a 66 matrix of lists.

JohnCremona · 2016-02-04T19:15:43Z

comment:32

Yes that is the thing I was expecting. Note that there is another bot on which the test passes. It's something nondeterministic in how I sort the curves in an isogeny class.

JohnCremona · 2016-02-04T20:31:04Z

comment:33

By the, this (sometimes) failing doctest has nothing whatsoever to do with the changes on this ticket, but there is some randomness creeping into the sorting of curves.

fchapoton · 2016-02-04T20:56:23Z

comment:34

I have found a typo (several times):

infintely many primes

Otherwise looks good to me (but I am no expert)

JohnCremona · 2016-02-09T14:55:15Z

comment:36

Would people be bothered if I marked the two strange doctests as "random" for the time being? Otherwise this sorting issue is delaying the merging of a genuine bug-fix. I have a strong personal incentive to get the sorting issue dealt with, but that should not be on this ticket anyway.

I plan to update the branch accordingly.

jdemeyer · 2016-02-09T14:57:18Z

comment:37

Replying to @JohnCremona:

Would people be bothered if I marked the two strange doctests as "random" for the time being?

With proper documentation and a reference to the discussion, no problem.

JohnCremona · 2016-02-09T16:41:38Z

comment:38

Replying to @jdemeyer:

Replying to @JohnCremona:

Would people be bothered if I marked the two strange doctests as "random" for the time being?

With proper documentation and a reference to the discussion, no problem.

OK, I'll see if I can meet your high standards or "proper"!

I corrected all occurrences of "infintely" also (2 here, but a few a other places).

sagetrac-git · 2016-02-09T16:49:29Z

Branch pushed to git repo; I updated commit sha1. New commits:

`226821f`	`corrected typo infinite(ly) in a few places`
`8680baa`	`made two doctests random pending #20028`

sagetrac-git · 2016-02-09T16:49:29Z

Changed commit from de3fdf1 to 8680baa

JohnCremona · 2016-02-09T16:50:03Z

comment:40

I opened a new ticket #20028 for the issue of sorting number field elements. Th ebranch now marks thoe tests as #random and refers to the discussion on this ticket.

New commits:

`226821f`	`corrected typo infinite(ly) in a few places`
`8680baa`	`made two doctests random pending #20028`

fchapoton · 2016-02-09T21:01:48Z

Reviewer: Frédéric Chapoton

fchapoton · 2016-02-09T21:01:48Z

comment:41

ok, good enough for me

vbraun · 2016-02-10T08:20:59Z

Changed branch from u/cremona/19229 to 8680baa

JohnCremona added this to the sage-6.9 milestone Sep 17, 2015

JohnCremona added c: elliptic curves labels Sep 17, 2015

This comment has been minimized.

Sign in to view

JohnCremona added the s: needs review label Sep 18, 2015

This comment has been minimized.

Sign in to view

jdemeyer modified the milestones: sage-6.9, sage-6.10 Dec 9, 2015

jdemeyer added s: needs work and removed s: needs review labels Dec 9, 2015

JohnCremona removed the s: needs work label Dec 9, 2015

fchapoton added s: needs work and removed s: needs review labels Feb 3, 2016

JohnCremona added s: needs review and removed s: needs work labels Feb 4, 2016

fchapoton modified the milestones: sage-6.10, sage-7.1 Feb 4, 2016

fchapoton added s: positive review and removed s: needs review labels Feb 9, 2016

vbraun removed the s: positive review label Feb 10, 2016

vbraun closed this as completed in 5313038 Feb 10, 2016

Bug in elliptic curve Galois Representation #19229

Bug in elliptic curve Galois Representation #19229

Comments

JohnCremona commented Sep 17, 2015

JohnCremona commented Sep 17, 2015

JohnCremona commented Sep 18, 2015

This comment has been minimized.

JohnCremona commented Sep 18, 2015

JohnCremona commented Sep 18, 2015

JohnCremona commented Sep 18, 2015

JohnCremona commented Sep 18, 2015

This comment has been minimized.

JohnCremona commented Dec 8, 2015

jdemeyer commented Dec 8, 2015

JohnCremona commented Dec 9, 2015

sagetrac-git mannequin commented Dec 9, 2015

sagetrac-git mannequin commented Dec 9, 2015

JohnCremona commented Dec 9, 2015

jdemeyer commented Dec 9, 2015

JohnCremona commented Dec 9, 2015

jdemeyer commented Dec 9, 2015

sagetrac-git mannequin commented Dec 9, 2015

sagetrac-git mannequin commented Dec 9, 2015

JohnCremona commented Dec 9, 2015

fchapoton commented Feb 3, 2016

JohnCremona commented Feb 3, 2016

sagetrac-git mannequin commented Feb 4, 2016

sagetrac-git mannequin commented Feb 4, 2016

JohnCremona commented Feb 4, 2016

fchapoton commented Feb 4, 2016

JohnCremona commented Feb 4, 2016

fchapoton commented Feb 4, 2016

JohnCremona commented Feb 4, 2016

JohnCremona commented Feb 4, 2016

fchapoton commented Feb 4, 2016

JohnCremona commented Feb 9, 2016

jdemeyer commented Feb 9, 2016

JohnCremona commented Feb 9, 2016

sagetrac-git mannequin commented Feb 9, 2016

sagetrac-git mannequin commented Feb 9, 2016

JohnCremona commented Feb 9, 2016

fchapoton commented Feb 9, 2016

fchapoton commented Feb 9, 2016

vbraun commented Feb 10, 2016