Opened 10 years ago

Closed 7 years ago

Last modified 7 years ago

#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 davidloeffler)

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)

trac_5048-sl2z_coercion.patch (8.8 KB) - added by davidloeffler 8 years ago.
patch against 4.6.1.alpha3
trac_5048-sl2z_coercion-rebased_for_10452.patch (8.8 KB) - added by davidloeffler 8 years ago.
Version that will apply happily after #10452
trac_5048-rebased_for_11422.patch (9.1 KB) - added by davidloeffler 8 years ago.
version rebased to 4.7.1.alpha4 + #11422
trac_5048-missingdoctest.patch (795 bytes) - added by davidloeffler 7 years ago.
apply over previous patch
trac_5048-missingdoctest.2.patch (795 bytes) - added by davidloeffler 7 years ago.
NOT NEEDED! See comments

Download all attachments as: .zip

Change History (30)

comment:1 Changed 10 years ago by AlexGhitza

  • Type changed from defect to enhancement

comment:2 Changed 10 years ago by davidloeffler

  • Component changed from number theory to modular forms
  • Owner changed from was to craigcitro

Changed 8 years ago by davidloeffler

patch against 4.6.1.alpha3

comment:3 Changed 8 years ago by davidloeffler

  • Authors set to David Loeffler
  • 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).

Changed 8 years ago by davidloeffler

Version that will apply happily after #10452

comment:4 Changed 8 years ago by davidloeffler

I just realised that the patch I just uploaded conflicts, in a rather trivial way, with the patch at #10452. The second patch will apply on top of the patch at #10452. The two patches are identical other than one line of diff context.

comment:5 Changed 8 years ago by davidloeffler

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 davidloeffler

Apply trac_5048-sl2z_coercion-rebased_for_10452.patch Depends on #10452

comment:7 Changed 8 years ago by davidloeffler

  • Priority changed from major to minor

Changed 8 years ago by davidloeffler

version rebased to 4.7.1.alpha4 + #11422

comment:8 Changed 8 years ago by davidloeffler

I've uploaded a version that is rebased to apply happily over the positively-reviewed patch #11422. This is intended to form part of a series #10335 - #11422 - #11598 - this ticket - #10453 -

#11601; but the patch itself is independent of #11598 and of the tickets later in the series.

comment:9 Changed 8 years ago by davidloeffler

  • 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 GeorgSWeber

  • Cc GeorgSWeber added

comment:11 Changed 8 years ago by GeorgSWeber

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 davidloeffler

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 davidloeffler

  • Description modified (diff)

comment:14 Changed 8 years ago by davidloeffler

  • Description modified (diff)

Georg: did you get any chance to look at this again? Since #11422 is still waiting around, I've clarified the description to point out that both the pre-#11422 and post-#11422 patches exist and are equally valid.

comment:15 Changed 8 years ago by davidloeffler

  • Description modified (diff)

comment:16 Changed 7 years ago by johanbosman

  • 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 GeorgSWeber

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 davidloeffler

  • 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?

Changed 7 years ago by davidloeffler

apply over previous patch

comment:19 Changed 7 years ago by davidloeffler

  • Description modified (diff)
  • Reviewers changed from Johan Bosman to Johan Bosman, Georg S. Weber
  • Status changed from needs_work to needs_review

Changed 7 years ago by davidloeffler

NOT NEEDED! See comments

comment:20 follow-up: Changed 7 years ago by davidloeffler

  • 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 johanbosman

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 saraedum

  • Keywords sd35 added

comment:23 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-4.8 to sage-5.0

comment:24 Changed 7 years ago by jdemeyer

  • 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 davidloeffler

Apply trac_5048-rebased_for_11422.patch

(for the patchbot, so it can understand the prerequisites for #11709)

Note: See TracTickets for help on using tickets.