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 mraum)

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:

  1. #10451

Apply:

  1. trac_10553.patch

Attachments (2)

10553-DiamondBrackedSpeedup (2.1 KB) - added by mderickx 7 years ago.
trac_10553.patch (807 bytes) - added by davidloeffler 7 years ago.
New patch. Requires #10451. Replaces previous patch.

Download all attachments as: .zip

Change History (9)

Changed 7 years ago by mderickx

comment:1 Changed 7 years ago by mderickx

  • Status changed from new to needs_review

comment:2 Changed 7 years ago by mderickx

  • Status changed from needs_review to needs_work

comment:3 Changed 7 years ago by mderickx

  • Status changed from needs_work to needs_review

comment:4 Changed 7 years ago by davidloeffler

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.

Changed 7 years ago by davidloeffler

New patch. Requires #10451. Replaces previous patch.

comment:5 Changed 7 years ago by davidloeffler

  • Authors set to Maarten Derickx, David Loeffler
  • 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 mraum

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

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