#5048 closed enhancement (fixed)
congruence subgroups are not integrated into the coercion model
Reported by: | ncalexan | Owned by: | craigcitro |
---|---|---|---|
Priority: | major | Milestone: | sage-5.0 |
Component: | modular forms | Keywords: | congruence subgroup coercion, sd35 |
Cc: | GeorgSWeber | Merged in: | sage-5.0.beta0 |
Authors: | David Loeffler | Reviewers: | Johan Bosman, Georg S. Weber |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
sage: Gamma0(10).1 * Gamma0(5).2 --------------------------------------------------------------------------- TypeError Traceback (most recent call last) /home/ncalexan/.sage/temp/sage.math.washington.edu/4030/_home_ncalexan__sage_init_sage_0.py in <module>() ----> 1 2 3 4 5 /scratch/nca/sage-3.3.alpha0-sage.math-only-x86_64-Linux/local/lib/python2.5/site-packages/sage/structure/element.so in sage.structure.element.MonoidElement.__mul__ (sage/structure/element.c:7375)() 849 850 --> 851 852 853 TypeError: unsupported operand parent(s) for '*': 'Congruence Subgroup Gamma0(10)' and 'Congruence Subgroup Gamma0(5)'
Apply
Attachments (5)
Change History (30)
comment:1 Changed 10 years ago by
- Type changed from defect to enhancement
comment:2 Changed 10 years ago by
- Component changed from number theory to modular forms
- Owner changed from was to craigcitro
Changed 8 years ago by
comment:3 Changed 8 years ago by
- Report Upstream set to N/A
- Status changed from new to needs_review
Here's a patch. I thought the simplest solution was to arrange that the parent of all congruence subgroup elements was always the (globally unique) SL2Z object, i.e. to make the various subgroup classes "facade parents" (like the prime integers example in the category docs).
comment:4 Changed 8 years ago by
comment:5 Changed 8 years ago by
Oh dear, it looks like the trac buildbot is trying to apply both patches at once, hence the red light! But it seems to have done one successful run with only the first patch installed.
comment:6 Changed 8 years ago by
Apply trac_5048-sl2z_coercion-rebased_for_10452.patch Depends on #10452
comment:7 Changed 8 years ago by
- Priority changed from major to minor
comment:8 Changed 8 years ago by
comment:9 Changed 8 years ago by
- Priority changed from minor to major
I'm raising the priority of this to "major" since it is a prerequisite for the (much more significant) patch #11601.
comment:10 Changed 8 years ago by
- Cc GeorgSWeber added
comment:11 Changed 8 years ago by
Hmmm, is there a quick and easy explanation for the change in the last line of the patch (file "sage/modular/modform/constructor.py")?
- 87 <class 'sage.modular.arithgroup.congroup_gamma0.Gamma0_class'>,
+ 87 <class 'sage.modular.arithgroup.congroup_gamma0.Gamma0_class_with_category'>,
I mean, this might be necessary, but due to the rest of this patch or due to some earlier patch??
comment:12 Changed 8 years ago by
It is necessary because the arithmetic subgroup init routine now calls Group.__init__
, which was not previously the case. The group init routine subsequently calls various other init routines that link arithmetic subgroups into Sage's category framework, with all of its baroque subtleties involving dynamic classes etc.
comment:13 Changed 8 years ago by
- Description modified (diff)
comment:14 Changed 8 years ago by
- Description modified (diff)
comment:15 Changed 8 years ago by
- Description modified (diff)
comment:16 Changed 7 years ago by
- Description modified (diff)
- Reviewers set to Johan Bosman
- Status changed from needs_review to positive_review
The patch still applies to 4.8.alpha4, the code looks sound, and all tests in sage.modular pass.
comment:17 Changed 7 years ago by
Hi Johan, thanks for looking at this one (I guess you're on SD 35 --- if so, I do envy you)!!
I dimly remember that this patch introduced a function with a missing doctest in "sage/modular/arithgroup/congroup_sl2z.py":
96 def call(self, x, check=True): 97 r""" 98 Create an element of self from x. If check=True (the default), check 99 that x really defines a 2x2 integer matrix of det 1. 100 """ 101 return ArithmeticSubgroupElement?(self, x, check=check) 102
I'm sorry I never got round to poke David about that (otherwise, the changes were OK for me) --- could you please have second look and check that independently ("sage --coverage ...modular/arithgroups/" or the like should show it)?
Thanks a lot in advance!
Cheers, Georg
comment:18 Changed 7 years ago by
- Status changed from positive_review to needs_work
Good catch, Georg! I will upload a patch shortly with the missing doctest, bringing doctest coverage in sage/modular back up to 100%. Georg, Johan: would either of you mind taking a quick look at the new patch?
comment:19 Changed 7 years ago by
- Description modified (diff)
- Reviewers changed from Johan Bosman to Johan Bosman, Georg S. Weber
- Status changed from needs_work to needs_review
comment:20 follow-up: ↓ 21 Changed 7 years ago by
- Description modified (diff)
- Status changed from needs_review to positive_review
Hang on a minute.
It is true that there is a doctest missing for this call method, but the missing doctest gets added in the next patch in the series (#11601). So I think we can ignore it here, since #11601 has a positive review; and I can safely reinstate Johan's positive review.
So: ignore the patch(es) I just uploaded -- it is not needed. I tried to obliterate the first patch by uploading a new blank patch on top of it, but made a hash of that. Perhaps it's time I went to bed.
comment:21 in reply to: ↑ 20 Changed 7 years ago by
Replying to davidloeffler:
Hang on a minute.
It is true that there is a doctest missing for this call method, but the missing doctest gets added in the next patch in the series (#11601). So I think we can ignore it here, since #11601 has a positive review; and I can safely reinstate Johan's positive review.
So: ignore the patch(es) I just uploaded -- it is not needed. I tried to obliterate the first patch by uploading a new blank patch on top of it, but made a hash of that. Perhaps it's time I went to bed.
Indeed. I was about to mention the same thing, but your comment crossed mine. :).
comment:22 Changed 7 years ago by
- Keywords sd35 added
comment:23 Changed 7 years ago by
- Milestone changed from sage-4.8 to sage-5.0
comment:24 Changed 7 years ago by
- Merged in set to sage-5.0.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
comment:25 Changed 7 years ago by
Apply trac_5048-rebased_for_11422.patch
(for the patchbot, so it can understand the prerequisites for #11709)
patch against 4.6.1.alpha3