Opened 7 years ago
Closed 7 years ago
#10553 closed enhancement (fixed)
Diamond bracket operators are terribly slow
Reported by: | mderickx | Owned by: | craigcitro |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7 |
Component: | modular forms | Keywords: | diamond operator |
Cc: | Merged in: | sage-4.7.alpha3 | |
Authors: | Maarten Derickx, David Loeffler | Reviewers: | Martin Raum |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
I was wondering why it took so much longer to get a diamond bracket operator on a cuspidal supspace then a hecke operator. I dived into it, and the difference is, that for hecke operators it is not checked if the cuspidal subspace really is a subspace but for diamond bracket operators it is checked.
The example below sows the possibillity of a large speed improvement since the theory already tells that the diamond bracket operators will also act on the cuspidal subspace.
sage: M=ModularSymbols(Gamma1(97),sign=1) sage: S=M.cuspidal_submodule() sage: d=M.diamond_bracket_operator(3).matrix() sage: time d.restrict(Smod,check=False) CPU times: user 0.42 s, sys: 0.08 s, total: 0.50 s Wall time: 0.68 s 345 x 345 dense matrix over Rational Field sage: time d.restrict(Smod,check=True) CPU times: user 74.42 s, sys: 0.52 s, total: 74.94 s Wall time: 75.42 s
Depends:
Apply:
Attachments (2)
Change History (9)
Changed 7 years ago by
comment:1 Changed 7 years ago by
- Status changed from new to needs_review
comment:2 Changed 7 years ago by
- Status changed from needs_review to needs_work
comment:3 Changed 7 years ago by
- Status changed from needs_work to needs_review
comment:4 Changed 7 years ago by
comment:5 Changed 7 years ago by
- Keywords diamond operator added
- Milestone set to sage-4.7
Depends on #10451 Apply trac_10553.patch
With #10451 installed it's just a case of adding "check=False" in one line of code. Hence the new patch. Obligatory timings:
# Before sage: S = ModularSymbols(Gamma1(97),sign=1).cuspidal_submodule() sage: time S.diamond_bracket_matrix(3) CPU times: user 62.15 s, sys: 0.03 s, total: 62.18 s Wall time: 62.17 s 345 x 345 dense matrix over Rational Field
# After sage: S = ModularSymbols(Gamma1(97),sign=1).cuspidal_submodule() sage: time S.diamond_bracket_matrix(3) CPU times: user 7.40 s, sys: 0.02 s, total: 7.42 s Wall time: 7.42 s 345 x 345 dense matrix over Rational Field
comment:6 Changed 7 years ago by
- Description modified (diff)
- Reviewers set to Martin Raum
- Status changed from needs_review to positive_review
That looks perfect; All tests pass.
comment:7 Changed 7 years ago by
- Merged in set to sage-4.7.alpha3
- Resolution set to fixed
- Status changed from positive_review to closed
Dear Maarten,
This is going to conflict rather severely with my patch at #10451, which (as well as adding all sorts of other functionality) changes the way diamond operators are implemented -- I made them into *elements* of a Hecke algebra, rather than the previous implementation as Hecke algebra *morphisms*, which I find quite strange!
I'd be really grateful if you wouldn't mind taking a look at #10451; if that gets a positive review, then we can do a new patch for this one depending on it, and I'll review it so both can go in the next release.