Opened 10 years ago

Closed 7 years ago

Last modified 3 years ago

#7922 closed enhancement (fixed)

Categories for Weyl character rings and weight rings (was: pickling fails in WeightRing)

Reported by: nthiery Owned by: bump
Priority: major Milestone: sage-4.7.1
Component: combinatorics Keywords:
Cc: bump, sage-combinat Merged in: sage-4.7.1.alpha2
Authors: Daniel Bump, Nicolas M. Thiéry Reviewers: Nicolas M. Thiéry, Dan Bump
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by nthiery)

This ticket refactors the code of WeylCharacterRing? and WeightRing? to use categories and (combinatorial) free modules. Along the way, it adds a couple features (Dan: please list them here), and solves a pickling issue which was caught by #7921:

sage: A2 = WeylCharacterRing(['A',2])
sage: a2 = WeightRing(A2)
sage: TestSuite(a2).run()
Failure in _test_element_pickling:
Traceback (most recent call last):
...
AssertionError: 2*a2(0,0,0) != 2*a2(0,0,0)

Indeed:

sage: x = a2.an_element()
sage: x == loads(dumps(x))
False

Apply trac_7922-rebased-4.7.alpha3.patch

Remove the following pickles from the pickle jar:

_class__sage_combinat_root_system_weyl_characters_WeightRing__.*
_class__sage_combinat_root_system_weyl_characters_WeylCharacterRing_class__.*
_class__sage_combinat_root_system_weyl_characters_WeylCharacter__.*

Copy the contents of trac_7922-new_pickles.tar.gz into data/ext_code/pickle_jar.

Attachments (9)

trac_7922.patch (85.5 KB) - added by bump 9 years ago.
Converts WeylCharacterRings? and WeightRings? to use category framework
trac_7922-rebased-4.6.1 (85.2 KB) - added by bump 9 years ago.
#7922: WeylCharacters? inherit from CombinatorialFreemodule? (substantial speedup)
trac_7922-doc.patch (10.9 KB) - added by bump 9 years ago.
7922: thematic tutorial revision
trac_7922-rebased-4.7.alpha1.patch (101.4 KB) - added by bump 9 years ago.
#7922: Revision of Weyl Character Rings
pickle_jar.tar.bz2 (606.2 KB) - added by bump 9 years ago.
#7922: revised pickle jar
pickle-notes (1.9 KB) - added by bump 9 years ago.
#7922: explanation of how the pickle jar is remade
trac_7922-rebased-4.7.alpha2.patch (120.9 KB) - added by bump 9 years ago.
#7922: revision of Weyl Character Rings
trac_7922-new_pickles.tar.gz (14.5 KB) - added by bump 9 years ago.
#7922: replacement pickles
trac_7922-rebased-4.7.alpha3.patch (136.4 KB) - added by bump 9 years ago.
#7922: Categories for Weyl character rings and weight rings

Download all attachments as: .zip

Change History (30)

comment:1 Changed 9 years ago by bump

  • Cc sage-combinat added
  • Description modified (diff)
  • Milestone set to sage-4.6
  • Reviewers set to nthiery
  • Status changed from new to needs_review

Regarding speed, testing a large number of branching rules show that the patch is a substantial speedup over the old code with a caveat: the old code has an option cache=true for WeylCharacterRings?. If this option (which is not the default) is selected for every WeylCharacterRing?, then the old code is faster. Typical times:

Old Code48 seconds
New Code25 seconds
Old Code, cache=true18 seconds

Since I made the method _irr_weights a cached method, the caching done in the new code is approximately equivalent to the caching in the old code, so at the moment I don't see any way to improve the situation.

comment:2 Changed 9 years ago by bump

Revised and reposted the patch in view of Nicolas' comment to use _from_dict(coerce=True).

Changed 9 years ago by bump

Converts WeylCharacterRings? and WeightRings? to use category framework

comment:3 Changed 9 years ago by bump

I uploaded a revised version of the patch. The only change is in classical_crystals.py, where the revision of WeylCharacterRings? necessitated a revision.

comment:4 Changed 9 years ago by bump

  • Milestone changed from sage-4.6 to sage-4.6.1

Requires rebuilding the pickle jar.

Changed 9 years ago by bump

#7922: WeylCharacters? inherit from CombinatorialFreemodule? (substantial speedup)

comment:5 Changed 9 years ago by bump

Since #9838 was merged in sage-4.6.1.alpha1, this patch needed rebasing.

I therefore posted trac_7922-rebased-4.6.1.

comment:6 Changed 9 years ago by bump

  • Milestone changed from sage-4.6.1 to sage-4.6.2

Changed 9 years ago by bump

7922: thematic tutorial revision

comment:7 Changed 9 years ago by bump

  • Description modified (diff)
  • Work issues set to pickle jar will have to be rebuilt

This patch slightly conflicts with #8442 which was merged. So I'm posting a second patch trac_7922-doc.patch which revises the thematic tutorial.

Changed 9 years ago by bump

#7922: Revision of Weyl Character Rings

comment:8 Changed 9 years ago by bump

  • Description modified (diff)

I have posted trac_7922-rebased-4.7.alpha1.patch, which addresses many of the comments in the reviewer patch:

http://combinat.sagemath.org/patches/file/tip/trac_7922-review-nt.patch

Those changes that are not addressed are discussed here:

http://groups.google.com/group/sage-combinat-devel/browse_thread/thread/277e146862632d72

Changed 9 years ago by bump

#7922: revised pickle jar

comment:9 Changed 9 years ago by bump

  • Description modified (diff)
  • Work issues pickle jar will have to be rebuilt deleted

Changed 9 years ago by bump

#7922: explanation of how the pickle jar is remade

comment:10 Changed 9 years ago by bump

See pickle-notes for an explanation of how the pickle jar was remade.

comment:11 Changed 9 years ago by bump

  • Description modified (diff)

comment:12 Changed 9 years ago by bump

  • Description modified (diff)

The revised patch trac_7922-rebased-4.7.alpha2 includes the revised coercion mechanism and other changes proposed by Nicolas.

In view of this message:

http://trac.sagemath.org/sage_trac/ticket/10354#comment:11

I posted a tarball containing only the new pickles, and a list of obsolete pickles to be discarded.

Changed 9 years ago by bump

#7922: revision of Weyl Character Rings

Changed 9 years ago by bump

#7922: replacement pickles

comment:13 Changed 9 years ago by nthiery

  • Authors set to Dan Bump, Nicolas M. Thiéry
  • Description modified (diff)
  • Reviewers changed from nthiery to Nicolas M. Thiéry, Dan Bump
  • Summary changed from Pickling fails in WeightRing to Categories for Weyl character rings and weight rings (was: pickling fails in WeightRing)
  • Type changed from defect to enhancement

Hi Dan,

I just did a final proofreading, fixed a couple typos, updated coerce_to_sl in the doctests of the thematic tutorials, and removed some trailing white space and tabs.

For me it is now all good to go. Please check my reviewer's patch on the sage-combinat patch server. If it's ok with you, you can fold/upload and set a positive review here on my behalf.

http://combinat.sagemath.org/patches/file/tip/trac_7922-final-review-nt.patch

Cheers,

Nicolas

comment:14 Changed 9 years ago by bump

  • Description modified (diff)

Changed 9 years ago by bump

#7922: Categories for Weyl character rings and weight rings

comment:15 Changed 9 years ago by nthiery

  • Description modified (diff)
  • Status changed from needs_review to positive_review

Hi Dan,

I just checked out your final changes on the Sage-Combinat queue (trac_7922_alpha3-changes.patch), and it looks all good. So the final rebased patch you just posted (and which includes the above) is good to go.

Positive review!

comment:16 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-4.7 to sage-4.7.1

comment:17 Changed 9 years ago by jdemeyer

  • Merged in set to sage-4.7.1.alpha2
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:18 follow-up: Changed 9 years ago by jdemeyer

  • Resolution fixed deleted
  • Status changed from closed to new

This needs a few small fixes:

  1. at various places doc/en/thematic_tutorials/lie/weyl_character_ring.rst, the indentation is inconsistent. It should be 4 spaces but in the newly added examples it is more or less random.
  2. in doc/en/thematic_tutorials/lie/weyl_character_ring.rst, the test on line 250 takes a very long time but is not marked as such.

Please add an additional patch fixing these issues.

comment:19 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.10 to sage-4.7.1
  • Resolution set to fixed
  • Status changed from new to closed

comment:20 in reply to: ↑ 18 Changed 7 years ago by nthiery

Replying to jdemeyer:

This needs a few small fixes:

  1. at various places doc/en/thematic_tutorials/lie/weyl_character_ring.rst, the indentation is inconsistent. It should be 4 spaces but in the newly added examples it is more or less random.
  2. in doc/en/thematic_tutorials/lie/weyl_character_ring.rst, the test on line 250 takes a very long time but is not marked as such.

Please add an additional patch fixing these issues.

Fixing the indentation is now #14678. The very long time is not necessary anymore, most likely thanks to the optimizations in #13391 (the example takes 0.22s on my machine).

comment:21 Changed 3 years ago by chapoton

  • Authors changed from Dan Bump, Nicolas M. Thiéry to Daniel Bump, Nicolas M. Thiéry
Note: See TracTickets for help on using tickets.