search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.41k stars 475 forks source link

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

Closed nthiery closed 11 years ago

nthiery commented 14 years ago

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 attachment: 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 attachment: trac_7922-new_pickles.tar.gz into data/ext_code/pickle_jar.

CC: @dwbump @sagetrac-sage-combinat

Component: combinatorics

Author: Daniel Bump, Nicolas M. Thiéry

Reviewer: Nicolas M. Thiéry, Dan Bump

Merged: sage-4.7.1.alpha2

Issue created by migration from https://trac.sagemath.org/ticket/7922

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 14 years ago

Description changed:

--- 
+++ 
@@ -18,5 +18,7 @@
 False

-I assume that this is an issue in equality test. This should be fixed for free when WeighRing's will use CombinatorialFreeModules +I assume that this is an issue in equality test. This should be fixed for free when WeightRing's will use CombinatorialFreeModules and categories. + +The patch trac_7922.patch fixes this bug by converting both WeylCharacterRings and WeightRings to inherit from CombinatorialFreeModule and to use the category framework, a change Nicolas Thiery has requested.

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 14 years ago
comment:1

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 Code 48 seconds
New Code 25 seconds
Old Code, cache=true 18 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.

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 14 years ago

Reviewer: nthiery

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 14 years ago
comment:2

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

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 14 years ago

Converts WeylCharacterRings and WeightRings to use category framework

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 14 years ago
comment:3

Attachment: trac_7922.patch.gz

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

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 14 years ago
comment:4

Requires rebuilding the pickle jar.

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 13 years ago

Attachment: trac_7922-rebased-4.6.1.gz

7922: WeylCharacters inherit from CombinatorialFreemodule (substantial speedup)

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 13 years ago
comment:5

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

I therefore posted trac_7922-rebased-4.6.1.

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 13 years ago

7922: thematic tutorial revision

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 13 years ago

Work Issues: pickle jar will have to be rebuilt

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 13 years ago

Description changed:

--- 
+++ 
@@ -21,4 +21,9 @@
 I assume that this is an issue in equality test. This should be fixed for free when WeightRing's will use CombinatorialFreeModules
 and categories.

-The patch trac_7922.patch fixes this bug by converting both WeylCharacterRings and WeightRings to inherit from CombinatorialFreeModule and to use the category framework, a change Nicolas Thiery has requested.
+Apply:
+
+```
+ trac_7922-rebased-4.6.1
+ trac_7922-doc.patch
+```
6bbcde06-8197-41f1-b9a3-c998bb839000 commented 13 years ago
comment:7

Attachment: trac_7922-doc.patch.gz

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.

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 13 years ago

Attachment: trac_7922-rebased-4.7.alpha1.patch.gz

7922: Revision of Weyl Character Rings

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 13 years ago
comment:8

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

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 13 years ago

Description changed:

--- 
+++ 
@@ -21,9 +21,8 @@
 I assume that this is an issue in equality test. This should be fixed for free when WeightRing's will use CombinatorialFreeModules
 and categories.

-Apply:
+Apply only
6bbcde06-8197-41f1-b9a3-c998bb839000 commented 13 years ago

7922: revised pickle jar

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 13 years ago

Description changed:

--- 
+++ 
@@ -21,8 +21,6 @@
 I assume that this is an issue in equality test. This should be fixed for free when WeightRing's will use CombinatorialFreeModules
 and categories.

-Apply only
+**Apply** [attachment: trac_7922-rebased-4.7.alpha1.patch]

-```
- trac_7922-rebased-4.7.alpha1.patch
-```
+**Copy** [attachment: pickle_jar.tar.bz2] into `data/ext_code/pickle_jar`.
6bbcde06-8197-41f1-b9a3-c998bb839000 commented 13 years ago
comment:9

Attachment: pickle_jar.tar.bz2.gz

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 13 years ago

Changed work issues from pickle jar will have to be rebuilt to none

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 13 years ago

Attachment: pickle-notes.gz

7922: explanation of how the pickle jar is remade

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 13 years ago
comment:10

See [attachment: pickle-notes] for an explanation of how the pickle jar was remade.

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 13 years ago

Description changed:

--- 
+++ 
@@ -21,6 +21,12 @@
 I assume that this is an issue in equality test. This should be fixed for free when WeightRing's will use CombinatorialFreeModules
 and categories.

-**Apply** [attachment: trac_7922-rebased-4.7.alpha1.patch]
+**Apply** [attachment: trac_7922-rebased-4.7.alpha2.patch]

-**Copy** [attachment: pickle_jar.tar.bz2] into `data/ext_code/pickle_jar`.
+**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 [attachment: new_pickles.tar.gz] into `data/ext_code/pickle_jar`.
6bbcde06-8197-41f1-b9a3-c998bb839000 commented 13 years ago
comment:12

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

In view of this message:

#10354 comment:11

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

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 13 years ago

Description changed:

--- 
+++ 
@@ -29,4 +29,5 @@
 _class__sage_combinat_root_system_weyl_characters_WeylCharacterRing_class__.*
 _class__sage_combinat_root_system_weyl_characters_WeylCharacter__.*

-**Copy** the contents of [attachment: new_pickles.tar.gz] into `data/ext_code/pickle_jar`.
+**Copy** the contents of 
+[attachment: trac_7922-new_pickles.tar.gz] into `data/ext_code/pickle_jar`.
6bbcde06-8197-41f1-b9a3-c998bb839000 commented 13 years ago

7922: revision of Weyl Character Rings

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 13 years ago

Attachment: trac_7922-rebased-4.7.alpha2.patch.gz

7922: replacement pickles

nthiery commented 13 years ago
comment:13

Attachment: trac_7922-new_pickles.tar.gz

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

nthiery commented 13 years ago

Changed reviewer from nthiery to Nicolas M. Thiéry, Dan Bump

nthiery commented 13 years ago

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
-First issue caught by #7921:
+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]) @@ -18,8 +18,6 @@ False


-I assume that this is an issue in equality test. This should be fixed for free when WeightRing's will use CombinatorialFreeModules
-and categories.

 **Apply** [attachment: trac_7922-rebased-4.7.alpha2.patch]

@@ -31,3 +29,4 @@

 **Copy** the contents of 
 [attachment: trac_7922-new_pickles.tar.gz] into `data/ext_code/pickle_jar`.
+
nthiery commented 13 years ago

Author: Dan Bump, Nicolas M. Thiéry

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 13 years ago

Description changed:

--- 
+++ 
@@ -19,13 +19,15 @@

-Apply [attachment: trac_7922-rebased-4.7.alpha2.patch] +Apply [attachment: 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 [attachment: trac_7922-new_pickles.tar.gz] into data/ext_code/pickle_jar.

6bbcde06-8197-41f1-b9a3-c998bb839000 commented 13 years ago

7922: Categories for Weyl character rings and weight rings

nthiery commented 13 years ago

Description changed:

--- 
+++ 
@@ -19,7 +19,7 @@

-Apply [attachment: trac_7922-rebased-4.7.alpha3.patch] +Apply attachment: trac_7922-rebased-4.7.alpha3.patch

Remove the following pickles from the pickle jar:

@@ -30,5 +30,5 @@


 **Copy** the contents of 
-[attachment: trac_7922-new_pickles.tar.gz] into `data/ext_code/pickle_jar`.
+[attachment: trac_7922-new_pickles.tar.gz](https://github.com/sagemath/sage/files/ticket7922/trac_7922-new_pickles.tar.gz.gz) into `data/ext_code/pickle_jar`.
nthiery commented 13 years ago
comment:15

Attachment: trac_7922-rebased-4.7.alpha3.patch.gz

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!

jdemeyer commented 13 years ago

Merged: sage-4.7.1.alpha2

jdemeyer commented 13 years ago
comment:18

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.

nthiery commented 11 years ago
comment:20

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

fchapoton commented 8 years ago

Changed author from Dan Bump, Nicolas M. Thiéry to Daniel Bump, Nicolas M. Thiéry