Opened 7 years ago

Closed 6 years ago

#9651 closed enhancement (fixed)

Addition on CombinatorialFreeModule directly on dictionaries

Reported by: stumpc5 Owned by: sage-combinat
Priority: major Milestone: sage-4.6.1
Component: combinatorics Keywords: addition of dictionaries, CombinatorialFreeModule
Cc: Merged in: sage-4.6.1.alpha0
Authors: Christian Stump Reviewers: Daniel Bump
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by bump)

At the moment, adding (and taking negative, substracting,...) for CombinatorialFreeModule? is not done in a very efficient way.

This patch

  • provides a cythonized version of a pointwise addition of dictionaries, together with multiple options
  • is (indirectly) based on the patch in Ticket #9648

======================================

prerequisite: trac_9648_modulemorphism_codomain_extension.2.patch

apply that patch and trac_9651_CombinatorialFreeModule_Addition-cs.4.patch, which supercedes the previously posted patches on this page.

Attachments (6)

trac_9651_CombinatorialFreeModule_Addition.patch (20.9 KB) - added by stumpc5 7 years ago.
trac_9651_CombinatorialFreeModule_Addition-cs.patch (21.4 KB) - added by stumpc5 7 years ago.
trac_9651_CombinatorialFreeModule_Addition-cs.2.patch (25.9 KB) - added by stumpc5 6 years ago.
trac_9651_CombinatorialFreeModule_Addition-cs.3.patch (26.4 KB) - added by stumpc5 6 years ago.
includes referees suggestions
trac_9651_CombinatorialFreeModule_Addition-cs.4.patch (26.3 KB) - added by stumpc5 6 years ago.
includes more referees suggestions
trac_9651_CombinatorialFreeModule_Addition-cs.5.patch (26.3 KB) - added by stumpc5 6 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 7 years ago by stumpc5

  • Status changed from new to needs_review

The patch still needs some doctesting (I have not yet done much, but will do more these days) - in particular, I modified CombinatorialFreeModule?._apply_module_morphism and .apply_module_endomorphism. The first method is used in the code for symmetric functions: powersum.py, sfa.py and macdonald.py. Would be nice if someone could check if everything there still works well.

Thx, Christian

comment:2 Changed 7 years ago by bump

The patch doesn't apply cleanly to sage-4.5.3.rc0. Applying #9648 first doesn't help.

comment:3 Changed 7 years ago by bump

The revised patch trac_9651_CombinatorialFreeModule_Addition-cs.patch applies cleanly to sage 4.6.alpha1 and passes all tests.

But I haven't been able to confirm that it is a speedup. The results of some testing are here:

http://groups.google.com/group/sage-combinat-devel/msg/4869cc885ca42bc1

Changed 6 years ago by stumpc5

includes referees suggestions

Changed 6 years ago by stumpc5

includes more referees suggestions

comment:4 Changed 6 years ago by bump

  • Description modified (diff)
  • Reviewers set to Daniel Bump
  • Status changed from needs_review to positive_review

Positive review!

There is a thread about this patch in sage-combinat-devel resulting in the latest version.

I tested this with sage-4.6.alpha3. After applying

trac_9648_modulemorphism_codomain_extension.2.patch 
trac_9651_CombinatorialFreeModule_Addition-cs.4.patch

all tests pass. I also confirmed that the test is a speedup.

comment:5 Changed 6 years ago by jdemeyer

  • Milestone set to sage-4.6.1

comment:6 follow-up: Changed 6 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Please remove * * * from the commit message

comment:7 in reply to: ↑ 6 Changed 6 years ago by stumpc5

  • Status changed from needs_work to positive_review

Replying to jdemeyer:

Please remove * * * from the commit message

done!

comment:8 Changed 6 years ago by jdemeyer

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