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: sage-8.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:

Status badges

Description (last modified by chapoton)

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 chapoton

  • Branch set to u/chapoton/23656
  • Commit set to 35109f23ac76af245385170d957b2e3b4ecec6bd
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

35109f2slightly better normalize for gamma_H congruence groups

comment:2 Changed 4 years ago by git

  • Commit changed from 35109f23ac76af245385170d957b2e3b4ecec6bd to dc152c51496d96beb6cbf1b0618edd29fd0561e1

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

dc152c5trac 23656 fixing doctests

comment:3 Changed 4 years ago by chapoton

bot is morally green, please review

comment:4 Changed 4 years ago by chapoton

  • Cc cremona roed added

green bot, please review (easy)

comment:5 Changed 4 years ago by vdelecroix

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 vdelecroix

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 vdelecroix

(though there is optimized code paths for small Zm but I don't think it matters here)

comment:8 Changed 4 years ago by vdelecroix

(you can basically get rid of Zm in your code)

comment:9 Changed 4 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to needs_work

comment:10 Changed 4 years ago by git

  • Commit changed from dc152c51496d96beb6cbf1b0618edd29fd0561e1 to 2bd381970acfeaa9d703b09e67e115df2791f368

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

581adefMerge branch 'u/chapoton/23656' in 8.1.b3
2bd3819trac 23656 reviewer's suggested changes

comment:11 Changed 4 years ago by chapoton

  • Status changed from needs_work to needs_review

Done, thanks for the review


New commits:

581adefMerge branch 'u/chapoton/23656' in 8.1.b3
2bd3819trac 23656 reviewer's suggested changes

comment:12 Changed 4 years ago by vdelecroix

    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 git

  • Commit changed from 2bd381970acfeaa9d703b09e67e115df2791f368 to 32208f403b152cf52bcdb86f76165ede5c74ab7b

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

32208f4trac 23656 detail (set)

comment:14 Changed 4 years ago by chapoton

done, thanks

comment:15 Changed 4 years ago by vdelecroix

  • Status changed from needs_review to positive_review

comment:16 Changed 4 years ago by mderickx

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 vbraun

  • Branch changed from u/chapoton/23656 to 32208f403b152cf52bcdb86f76165ede5c74ab7b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.