Ticket #6425 (closed defect: fixed)

Opened 4 years ago

Last modified 3 years ago

GammaH constructor doesn't check the gens are units mod N

Reported by: davidloeffler Owned by: AlexGhitza
Priority: major Milestone: sage-4.3.1
Component: modular forms Keywords: congruence subgroups
Cc: Work issues:
Report Upstream: N/A Reviewers: John Cremona
Authors: Alex Ghitza Merged in: sage-4.3.1.alpha0
Dependencies: Stopgaps:

Description

This says it all, I think: 

sage: GammaH(14,[10])
Congruence Subgroup Gamma_H(14) with H generated by [10]

Attachments

trac_6425.patch Download (3.1 KB) - added by AlexGhitza 3 years ago.

Change History

comment:1 Changed 4 years ago by AlexGhitza

  • Owner changed from davidloeffler to AlexGhitza
  • Status changed from new to assigned
  • Summary changed from GammaH constructor doesn't check the gens are units mod N to [with patch, needs review] GammaH constructor doesn't check the gens are units mod N
  • Authors set to Alex Ghitza

comment:2 Changed 4 years ago by cremona

  • Reviewers set to John Cremona
  • Summary changed from [with patch, needs review] GammaH constructor doesn't check the gens are units mod N to [with patch, with review and suggestion] GammaH constructor doesn't check the gens are units mod N

This looks fine, it applies to 4.1.1 and the tests (in the directory modular/arithgroup) pass.

Suggestion: there is already a function _normalize_H(H, level) which checks that H is a list, reduces its entries modulo the level, sorts and deletes 1 if present. Why not add the gcd test in there? I would not mak a lot of difference, but the code would be tidier that way.

comment:3 Changed 3 years ago by AlexGhitza

  • Status changed from needs_work to needs_review
  • Report Upstream set to N/A
  • Summary changed from [with patch, with review and suggestion] GammaH constructor doesn't check the gens are units mod N to GammaH constructor doesn't check the gens are units mod N

John, you are of course right. I have made the change and replaced the patch.

Changed 3 years ago by AlexGhitza

comment:4 Changed 3 years ago by cremona

  • Status changed from needs_review to positive_review

Looks good -- applies to 4.3 and tests (as above) pass.

comment:5 Changed 3 years ago by mhansen

  • Status changed from positive_review to closed
  • Resolution set to fixed
  • Merged in set to sage-4.3.1.alpha0
Note: See TracTickets for help on using tickets.