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

Add a more efficient implementation of index for Gamma(N). #6606

Closed
simon-king-jena opened this issue Jul 23, 2009 · 10 comments
Closed

Add a more efficient implementation of index for Gamma(N). #6606

simon-king-jena opened this issue Jul 23, 2009 · 10 comments

Comments

@simon-king-jena
Copy link
Member

Gamma(N).index used the default implementation which was slow. Attached is a new implementation which works for the specific subgroup.

CC: @roed314

Component: modular forms

Author: Simon Morris

Reviewer: John Cremona, David Roed

Merged: Sage 4.1.1.alpha1

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

@simon-king-jena
Copy link
Member Author

Attachment: gamma.patch.gz

@JohnCremona
Copy link
Member

comment:1

The code loooked (though I would have written p**(3e-2)(p*p-1) ) but after applying to 4.1:

sage: Gamma(19).index()
---------------------------------------------------------------------------
NameError                                 Traceback (most recent call last)

/home/john/.sage/temp/ubuntu/25083/_home_john__sage_init_sage_0.py in <module>()

/home/john/sage-4.1/local/lib/python2.6/site-packages/sage/modular/arithgroup/congroup_gamma.pyc in index(self)
    105             32893086819240
    106         """
--> 107         return prod([p**(3*e) - p**(3*e-2) for (p,e) in self.level().factor()])
    108 

Looks like someone forgot to run sage -t before submitting the patch...

@JohnCremona
Copy link
Member

Reviewer: John Cremona

@simon-king-jena
Copy link
Member Author

comment:2

Attachment: gamma.2.patch.gz

@roed314
Copy link
Contributor

roed314 commented Jul 24, 2009

comment:3

I ran sage -t after applying the patch, and all tests pass. Looks good to me.

@JohnCremona
Copy link
Member

comment:4

That's better!

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Jul 24, 2009

comment:5

Simon: The patch gamma.2.patch doesn't contain your username. I've committed it in your name.

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Jul 24, 2009

Changed reviewer from John Cremona to John Cremona, David Roed

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Jul 24, 2009

Changed author from simon to Simon Morris

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Jul 24, 2009

Merged: Sage 4.1.1.alpha1

@sagetrac-mvngu sagetrac-mvngu mannequin closed this as completed Jul 24, 2009
@sagetrac-mvngu sagetrac-mvngu mannequin reopened this Jul 24, 2009
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