Ticket #4842 (closed defect: fixed)

Opened 21 months ago

Last modified 21 months ago

[with patch, with positive review] Fix performance regression in eisenstein_submodule.py due to cyclotomic coercion

Reported by: mabshoff Owned by: robertwb
Priority: critical Milestone: sage-3.2.3
Component: coercion Keywords:
Cc: craigcitro, robertwb Author(s):
Report Upstream: Reviewer(s):
Merged in: Work issues:

Description

See  http://groups.google.com/group/sage-devel/browse_thread/thread/12394b2efb1f6344/59421c4079e00cc5 for details: 

> That example was with CyclotomicField(12) and CyclotomicField(132) ... 

Ah. I bet the time was spent resolving the roots of CyclotomicField 
(132) to high enough precision to distinguish them. If you don't come   
up with a patch for this, I'll (probably) do it later tonight. 
- Robert

Cheers,

Michael

Attachments

4842-cyclo-coerce.patch Download (12.2 KB) - added by robertwb 21 months ago.

Change History

Changed 21 months ago by robertwb

  • cc robertwb added

Changed 21 months ago by robertwb

  • summary changed from Fix performance regression in eisenstein_submodule.py to [with patch, needs review] Fix performance regression in eisenstein_submodule.py

Changed 21 months ago by robertwb

  • summary changed from [with patch, needs review] Fix performance regression in eisenstein_submodule.py to [with patch, not ready for review] Fix performance regression in eisenstein_submodule.py

Oh, I should handle the case of 2m -> m for m odd.

Changed 21 months ago by robertwb

Changed 21 months ago by robertwb

  • summary changed from [with patch, not ready for review] Fix performance regression in eisenstein_submodule.py to [with patch, needs review] Fix performance regression in eisenstein_submodule.py due to cyclotomic coercion

This took longer than I expected due to build issues I ran into, but here's the patch. It should cover all cases, and use your fast code when the orders divide each other.

Changed 21 months ago by mabshoff

For the record: Patch applies, builds fine and all doctests with -long pass. The performance regression seems to have been fixed, i.e. before: 

sage -t -long "devel/sage/sage/modular/modform/eisenstein_submodule.py"
	 [73.3 s]

After the patch: 

sage -t -long "devel/sage/sage/modular/modform/eisenstein_submodule.py"
	 [3.4 s]

Cheers,

Michael

Changed 21 months ago by cremona

  • summary changed from [with patch, needs review] Fix performance regression in eisenstein_submodule.py due to cyclotomic coercion to [with patch, with positive review] Fix performance regression in eisenstein_submodule.py due to cyclotomic coercion

This looks very good to me. The maths is sound, the examples I tried worked, doctests passed and Michael is happy -- what more could we want? I give this a positive review.

Changed 21 months ago by mabshoff

  • status changed from new to closed
  • resolution set to fixed

Merged in Sage 3.2.3.alpha0

Note: See TracTickets for help on using tickets.