Opened 12 years ago

Closed 11 years ago

Last modified 9 years ago

#8998 closed defect (fixed)

galois_action on cusps has a bug

Reported by: was Owned by: craigcitro
Priority: major Milestone: sage-4.7
Component: modular forms Keywords:
Cc: Merged in: sage-4.7.alpha5
Authors: William Stein Reviewers: John Cremona
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

Ticket #5822 implemented the action of Galois on cusps. I think the algorithm was only designed to work for Gamma_0(N). However, the code runs for other groups, and doesn't raise an error. Unfortunately, it gives completely wrong results in some cases, e.g.,

sage: G = Gamma1(19)
sage: rational_cusps = [c for c in G.cusps() if c.galois_action(2,19).is_gamma1_equiv(c,19)]
sage: rational_cusps
[0, 2/19, 1/9, 1/8, 1/7, 3/19, 1/6, 1/5, 4/19, 1/4, 5/19, 6/19, 1/3,
7/19, 8/19, 9/19, 1/2, Infinity]

However, exactly half the cusps are rational (see, e.g., my paper http://wstein.org/papers/j1p/ or the work of Kubert-Lang).

This came up in research that Michael Stoll and I were doing, and it was temporarily very confusing.

Attachments (1)

trac_8998.patch (1.3 KB) - added by was 11 years ago.

Download all attachments as: .zip

Change History (8)

Changed 11 years ago by was

comment:1 Changed 11 years ago by was

  • Status changed from new to needs_review

It turns out that cusps (P1(Q)) don't know their group of course. This function is thus somewhat badly named. A minimal invasive change without breaking backwards compatibility is to clarify the *documentation* and do nothing else. I think that is enough to close this ticket, though I may want to make another ticket later to add other actions, etc. But for now, it is certainly a big improvement to at least fix a major incorrect statement in the documentation! This patch is pretty safe, since it *only* changes documentation.

comment:2 follow-up: Changed 11 years ago by cremona

Looks good -- testing now.

comment:3 in reply to: ↑ 2 Changed 11 years ago by cremona

  • Authors set to William Stein
  • Reviewers set to John Cremona
  • Status changed from needs_review to positive_review

Replying to cremona:

Looks good -- testing now.

OK, tests passed.

comment:4 Changed 11 years ago by jdemeyer

  • Merged in set to sage-4.7.alpha5
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:5 Changed 9 years ago by mderickx

This ticket is a false ticket. I just looked in to the article and the code in the article is really general. The problem is that is_gamma1_equiv does not do what you expect it to!

if Cusp(0).is_gamma1_equiv(Cusp(infinity),101):
    print "You would not expect to see something printed"

Actually prints the above statement. The reason for this is that is_gamma1_equiv does not return a bool but a tuple. The first element of this tuple is the bool you were actually looking for.

By the way, I find the claim in this ticket that the code of 5882 only works for Gamma0(N) quite stupid because for Gamma0(n) one might as well implement the following:

def galois_action(self,t,N):
    return self

Since all cusps on X_0(N).

However there is still a bug in the code because:

N=5
G=Gamma1(N)
for i in G.cusps():
    print [j.galois_action(2,N).is_gamma1_equiv(i,N)[0] for j in G.cusps()].count(True)

Prints

2
1
0
1

So the the galois action is not even a permutation!

What would be the right thing to do? Reopen this ticket, open a new one?

comment:6 Changed 9 years ago by cremona

Open a new one, with cross-referencing.

comment:7 Changed 9 years ago by mderickx

Ok, I created #13253 for this. And uploaded the patch I already created. It now needs review.

Note: See TracTickets for help on using tickets.