Opened 4 years ago
Closed 4 years ago
#23656 closed enhancement (fixed)
better normalize for Gamma_h congruence subgroups
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.1 
Component:  modular forms  Keywords:  
Cc:  mderickx, cremona, roed  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  32208f4 (Commits, GitHub, GitLab)  Commit:  32208f403b152cf52bcdb86f76165ede5c74ab7b 
Dependencies:  Stopgaps: 
Description (last modified by )
namely, do not use an element and its inverse as generators
This helps to fix partially #15341
Change History (17)
comment:1 Changed 4 years ago by
 Branch set to u/chapoton/23656
 Commit set to 35109f23ac76af245385170d957b2e3b4ecec6bd
 Description modified (diff)
 Status changed from new to needs_review
comment:2 Changed 4 years ago by
 Commit changed from 35109f23ac76af245385170d957b2e3b4ecec6bd to dc152c51496d96beb6cbf1b0618edd29fd0561e1
Branch pushed to git repo; I updated commit sha1. New commits:
dc152c5  trac 23656 fixing doctests

comment:3 Changed 4 years ago by
bot is morally green, please review
comment:5 Changed 4 years ago by
From an aesthetic point of view the following is ugly
sage: GammaH(11,[4]) Congruence Subgroup Gamma_H(11) with H generated by [3]
Could you change the 4 to the 3?
comment:6 Changed 4 years ago by
For (~Zm(h)).lift()
you have inverse_mod
sage: 10.inverse_mod(7) 5 sage: (~Zmod(7)(10)).lift() 5
comment:7 Changed 4 years ago by
(though there is optimized code paths for small Zm
but I don't think it matters here)
comment:8 Changed 4 years ago by
(you can basically get rid of Zm
in your code)
comment:9 Changed 4 years ago by
 Reviewers set to Vincent Delecroix
 Status changed from needs_review to needs_work
comment:10 Changed 4 years ago by
 Commit changed from dc152c51496d96beb6cbf1b0618edd29fd0561e1 to 2bd381970acfeaa9d703b09e67e115df2791f368
comment:11 Changed 4 years ago by
 Status changed from needs_work to needs_review
comment:12 Changed 4 years ago by
final_H = [] ... sorted(set(final_H))
Why don't you use a set from the begining final_H = set()
?
comment:13 Changed 4 years ago by
 Commit changed from 2bd381970acfeaa9d703b09e67e115df2791f368 to 32208f403b152cf52bcdb86f76165ede5c74ab7b
Branch pushed to git repo; I updated commit sha1. New commits:
32208f4  trac 23656 detail (set)

comment:14 Changed 4 years ago by
done, thanks
comment:15 Changed 4 years ago by
 Status changed from needs_review to positive_review
comment:16 Changed 4 years ago by
p.s. just in case people here are interested, there is now also a patch at #15341 that completely fixes the hashing problem and it now needs review
comment:17 Changed 4 years ago by
 Branch changed from u/chapoton/23656 to 32208f403b152cf52bcdb86f76165ede5c74ab7b
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
slightly better normalize for gamma_H congruence groups