Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Galois representations over number fields speedup #21776

Closed
JohnCremona opened this issue Oct 27, 2016 · 27 comments
Closed

Galois representations over number fields speedup #21776

JohnCremona opened this issue Oct 27, 2016 · 27 comments

Comments

@JohnCremona
Copy link
Member

The code for computing Galois representations of elliptic curves over number fields (which is used in isogeny computation) uses the default iterator over primes of degree 1 in the number field. Two problems: first, the method K.primes_of_degree_one_iter() only gives primes up to some norm bound, and that is not always large enough if left at the default. Second, where the function is used only principal primes are wanted but the iterator starts at 2 wheras there are no principal primes of norm less than discriminant/4!

I put in a custom iterator which helps a lot.

I will upload a patch when I have recovered an example which fails badly.

Component: elliptic curves

Keywords: Galois representations

Author: John Cremona

Branch/Commit: 7ca67b0

Reviewer: Travis Scrimshaw, Frédéric Chapoton

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

@JohnCremona
Copy link
Member Author

Branch: u/cremona/21776

@JohnCremona
Copy link
Member Author

Commit: aa7c953

@JohnCremona
Copy link
Member Author

New commits:

aa7c953#21776: speed up elliptic curve galois reps & isogenies

@tscrim
Copy link
Collaborator

tscrim commented Oct 27, 2016

comment:2

Two small things:

-    for l in D:
+    for l in D.iterkeys():

This change is unnecessary as the iteration over a dictionary is the keys (and iterkeys is going away in Python3).
Also, xrange is going away in Python3, so you should revert that change because the from six.moves import range already makes range an iterator, not a list.

@JohnCremona
Copy link
Member Author

comment:3

Thanks, will do. I have no idea why I put in the iterkeys().

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 27, 2016

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

41c708d#21776: minor changes after first review

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 27, 2016

Changed commit from aa7c953 to 41c708d

@fchapoton
Copy link
Contributor

comment:5

You should add documentation to the new "deg_one_prime_iter" function.

@JohnCremona
Copy link
Member Author

comment:6

OK, I am working on this.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 8, 2016

Changed commit from 41c708d to f203c24

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 8, 2016

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

fa2596c#21776: add docstring to new function
f203c24#21776: add doctest too

@JohnCremona
Copy link
Member Author

comment:8

Docstring and test added.

@tscrim
Copy link
Collaborator

tscrim commented Nov 8, 2016

Changed branch from u/cremona/21776 to u/tscrim/21776

@tscrim
Copy link
Collaborator

tscrim commented Nov 8, 2016

Changed commit from f203c24 to 39d9afb

@tscrim
Copy link
Collaborator

tscrim commented Nov 8, 2016

comment:9

Looking at the primes documentation, it says you can pass oo as the second argument. So I removed the maxnorm argument. I also did a few cosmetic changes. If you agree with my changes, then you can set a positive review.


New commits:

cfd653aMerge branch 'u/cremona/21776' of git://trac.sagemath.org/sage into u/tscrim/21776
39d9afbRemoving the maxnorm and some cosmetics.

@tscrim
Copy link
Collaborator

tscrim commented Nov 8, 2016

Reviewer: Travis Scrimshaw, Frédéric Chapoton

@fchapoton
Copy link
Contributor

comment:10

it.next() is not python3 compatible, use next(it) instead

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 8, 2016

Changed commit from 39d9afb to 0881746

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 8, 2016

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

0881746Fixing it.next() to next(it).

@JohnCremona
Copy link
Member Author

comment:12

Replying to @fchapoton:

it.next() is not python3 compatible, use next(it) instead

I did not know that -- and in fact one thing I had done was to make the reverse change, thinking it was somehow more efficient. Thanks.

I am happy with the changes since my commits. What next?

@fchapoton
Copy link
Contributor

comment:13

doc does not build, EXAMPLES: should be EXAMPLES::

@JohnCremona
Copy link
Member Author

comment:14

OK, I'll fix that and put it back into my branch... (building docs takes such a long time it discourages proper testing!)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 9, 2016

Changed commit from 0881746 to 7ca67b0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 9, 2016

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

7ca67b0Add the forgotten colon.

@tscrim
Copy link
Collaborator

tscrim commented Nov 9, 2016

comment:16

I just took care of it. (Partial doc builds can help for this kind of testing.) Feel free to put your branch if you make other changes.

@fchapoton
Copy link
Contributor

comment:17

ok, looks good enough.

@vbraun
Copy link
Member

vbraun commented Nov 17, 2016

Changed branch from u/tscrim/21776 to 7ca67b0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants