Opened 10 years ago
Closed 7 years ago
#9265 closed enhancement (fixed)
Remove `CombinatorialClass` from sage.combinat.tableau
Reported by: | jbandlow | Owned by: | sage-combinat |
---|---|---|---|
Priority: | major | Milestone: | sage-5.5 |
Component: | combinatorics | Keywords: | tableaux, combinatorics |
Cc: | Merged in: | sage-5.5.beta0 | |
Authors: | Jason Bandlow, Andrew Mathas | Reviewers: | Andrew Mathas, Anne Schilling |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #5457 | Stopgaps: |
Description (last modified by )
The CombinatorialClass
class is being deprecated. See Sage Combinat Roadmap for more information. This ticket will handle removing this class from sage.combinat.tableau. See also some discussion of this on this thread.
Apply: trac_9265_tableaux_categories_am.patch
and then trac_9265--tableaux_categories_pickles-am.patch
(and don't update the pickle jar!)
Attachments (2)
Change History (73)
comment:1 Changed 10 years ago by
- Milestone changed from sage-4.4.5 to sage-4.5
comment:2 Changed 9 years ago by
There is now a patch on the sage-combinat queue, which can be viewed here:
http://combinat.sagemath.org/hgwebdir.cgi/patches/file/tip/trac_9265_tableaux_categories_jb.patch
This needs some slight refactoring to apply to a clean 4.6.2, but anyone interested is very welcome to begin reviewing the patch and recording comments here. Thanks!
comment:3 Changed 9 years ago by
- Dependencies set to #10603, #11314
- Description modified (diff)
I'm not setting to 'needs review' since #10603 is a dependency and is not finalized. But other than that, it is ready to review in the current state. Comments welcome!
comment:4 Changed 9 years ago by
Updated the patch to reflect the discussion here.
comment:5 Changed 8 years ago by
- Status changed from new to needs_review
needs a (trivial, hopefully) rebase for Sage 5.2.rc0
comment:6 Changed 8 years ago by
- Status changed from needs_review to needs_work
comment:7 Changed 8 years ago by
I have rebased Jason's patch so that it apples to 5.2-rc0. I have to rename the patch as trac would not give me permission to delete some one else's patch.
I'll look at 5.2 soonish. The patch probably won't apply cleanly as for trivial reasons (white space) it does not commute with the two patches
trac_5457-symmetric_functions-mz.patch trac_12943-skew-partition-construction-bug-ht.patch
comment:8 Changed 8 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
Patch rebased so that it now applies cleanly to the top of sage 5.2.
comment:9 Changed 8 years ago by
- Description modified (diff)
comment:10 Changed 8 years ago by
For the patchbot:
Apply: trac_9265_tableaux_categories_am.patch
comment:11 Changed 8 years ago by
- Dependencies #10603, #11314 deleted
- Description modified (diff)
comment:12 Changed 8 years ago by
- Description modified (diff)
comment:13 Changed 8 years ago by
- Description modified (diff)
comment:14 Changed 8 years ago by
comment:15 follow-up: ↓ 16 Changed 8 years ago by
It looks like the colon after the "Apply" above is confusing the patchbot. So let's try:
Apply trac_9265_tableaux_categories_am.patch
comment:16 in reply to: ↑ 15 Changed 8 years ago by
Replying to andrew.mathas:
It looks like the colon after the "Apply" above is confusing the patchbot. So let's try:
Apply trac_9265_tableaux_categories_am.patch
Dear Andrew,
I just tried to apply the above patch to a clean version of sage-5.3.beta0 and got a failure
applying trac_9265_tableaux_categories_am.patch patching file sage/combinat/tableau.py Hunk #5 FAILED at 260 Hunk #6 succeeded at 268 with fuzz 2 (offset -4 lines). Hunk #40 FAILED at 4110 2 out of 40 hunks FAILED -- saving rejects to file sage/combinat/tableau.py.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh trac_9265_tableaux_categories_am.patch
If you do the same, you can look at sage/combinat/tableau.py.rej to see what the conflict is.
Anne
comment:17 Changed 8 years ago by
Hi Anne,
Thanks for this. I have just uploaded a new version of the patch which applies to the very top of 5.3 (the patch does not commute with #5457, but as it is applied first that doesn't matter).
In a previous version of the patch I (not Jason) had renamed some of the tableaux classes such as
- *Tableaux_n --> *Tableaux_size
- *Tableaux_p --> *Tableaux_shape
etc. but the current version does not do this. Personally I think that the name on the right make more sense but I probably shouldn't change this without consulting people first.
Andrew
--
For the patchbot:
Apply trac_9265_tableaux_categories_am.patch
--
The patchbot appears to be applying Jason's old patch before the new patch. I don't understand why this is happening but, as far as I can tell, this is why the patchbot is saying that the patch does not apply???
comment:18 follow-ups: ↓ 19 ↓ 33 Changed 8 years ago by
As the patchbot was complaining, I just uploaded a new version of the patch which deletes all trailing white space. Probably this is unwise as previously I used to have my editor do this automatically but I found that it meant that I had to rebase my patch all of the time so this will probably cause havoc further down the queue?
What is the accepted practise here?
comment:19 in reply to: ↑ 18 ; follow-up: ↓ 20 Changed 8 years ago by
Replying to andrew.mathas:
As the patchbot was complaining, I just uploaded a new version of the patch which deletes all trailing white space. Probably this is unwise as previously I used to have my editor do this automatically but I found that it meant that I had to rebase my patch all of the time so this will probably cause havoc further down the queue?
What is the accepted practise here?
Dear Andrew,
Did you actually upload the new version of the patch to the sage-combinat queue? I could not see it there. Usually trailing white spaces should be removed. But please check that the whole queue still applies (by trying hg qpush -a) in case you will post the patch there. If it causes many conflicts, it might be better not to remove them at this point.
Thanks,
Anne
comment:20 in reply to: ↑ 19 Changed 8 years ago by
Dear Andrew,
Here are a couple of comments on the ticket:
- Please add an extra line after lines 1872 and 3799
- Delete line 3271
- In line 3273 in getitem EXAMPLE: should be replaced by EXAMPLES:: (there is an S and : missing)
- In lines 3491, 3769, 4022, EXAMPLE:: should be replaced by EXAMPLES:: (there is an S missing)
- In line 4029, what is p? Perhaps
self
?
I think it would be ok if you replaced
- Tableaux_n --> Tableaux_size
- Tableaux_p --> Tableaux_shape
This is indeed more descriptive!
Anne
comment:21 follow-up: ↓ 22 Changed 8 years ago by
Hi Anne,
Sorry for the slow response. As you no doubt worked out I didn't push the previous patch onto the queue. I was avoiding doing this as I thought that moving the patch up in the queue, and rebasing to 5.3, would cause conflicts further down the queue.
Any way, after making in the changes that you suggested above I decided to push the new version onto the queue. I have made all of the changes that you requested. In changing the names of the classes to Tableaux_size etc I went a little further and changed self.n to self.size etc and changed the named arguments to the parent classes, although the old named arguments are still accepted.
Also, when I was fixing up the documentation error in getitem that you found I remembered that I wanted to rewrite getitem so that it supports slices. It turned out that each of the semistandard tableaux parent classes had its own getitem, so I removed all of these and added a generic getitem to the SemistandardTableaux? class which supports slices (the method came from my tableaux tuple classes). To make sure that this worked I put all of the doctests from the old getitems into the new generic getitem and then added a few more. To make the new getitem work I also had to add an is_finite method to these classes -- this probably should be implemented in the (In)FiniteEnumeratedSet? category, but it is not and I couldn't see were to add this there....
The new patch is both on trac and on the queue.
Andrew
--
For the patchbot:
Apply trac_9265_tableaux_categories_am.patch
comment:22 in reply to: ↑ 21 ; follow-ups: ↓ 23 ↓ 24 Changed 8 years ago by
Hi Andrew,
Thank you for your work on this. I ran sage -testall and got the following doctest failure:
sage -t devel/sage-sf/sage/structure/sage_object.pyx ********************************************************************** File "/Applications/sage-5.3.beta0/devel/sage-sf/sage/structure/sage_object.pyx", line 1114: sage: sage.structure.sage_object.unpickle_all() # (4s on sage.math, 2011) Expected: doctest:... DeprecationWarning: This class is replaced by Matrix_modn_dense_float/Matrix_modn_dense_double. See http://trac.sagemath.org/4260 for details. Successfully unpickled ... objects. Failed to unpickle 0 objects. Got: * unpickle failure: load('/Users/anne/.sage/temp/lolita_4.local/85064/dir_2/pickle_jar/_class__sage_combinat_crystals_affine_AffineCrystalFromClassicalAndPromotion_with_category_element_class__.sobj') * unpickle failure: load('/Users/anne/.sage/temp/lolita_4.local/85064/dir_2/pickle_jar/_class__sage_combinat_crystals_tensor_product_CrystalOfTableaux_with_category_element_class__.sobj') * unpickle failure: load('/Users/anne/.sage/temp/lolita_4.local/85064/dir_2/pickle_jar/_class__sage_combinat_crystals_tensor_product_TensorProductOfCrystalsWithGenerators_with_category__.sobj') * unpickle failure: load('/Users/anne/.sage/temp/lolita_4.local/85064/dir_2/pickle_jar/_class__sage_combinat_skew_tableau_SemistandardSkewTableaux_n__.sobj') * unpickle failure: load('/Users/anne/.sage/temp/lolita_4.local/85064/dir_2/pickle_jar/_class__sage_combinat_skew_tableau_SemistandardSkewTableaux_nmu__.sobj') * unpickle failure: load('/Users/anne/.sage/temp/lolita_4.local/85064/dir_2/pickle_jar/_class__sage_combinat_skew_tableau_SemistandardSkewTableaux_p__.sobj') * unpickle failure: load('/Users/anne/.sage/temp/lolita_4.local/85064/dir_2/pickle_jar/_class__sage_combinat_skew_tableau_SemistandardSkewTableaux_pmu__.sobj') * unpickle failure: load('/Users/anne/.sage/temp/lolita_4.local/85064/dir_2/pickle_jar/_class__sage_combinat_skew_tableau_StandardSkewTableaux_n__.sobj') * unpickle failure: load('/Users/anne/.sage/temp/lolita_4.local/85064/dir_2/pickle_jar/_class__sage_combinat_tableau_SemistandardTableaux_n__.sobj') * unpickle failure: load('/Users/anne/.sage/temp/lolita_4.local/85064/dir_2/pickle_jar/_class__sage_combinat_tableau_SemistandardTableaux_nmu__.sobj') * unpickle failure: load('/Users/anne/.sage/temp/lolita_4.local/85064/dir_2/pickle_jar/_class__sage_combinat_tableau_SemistandardTableaux_p__.sobj') * unpickle failure: load('/Users/anne/.sage/temp/lolita_4.local/85064/dir_2/pickle_jar/_class__sage_combinat_tableau_SemistandardTableaux_pmu__.sobj') * unpickle failure: load('/Users/anne/.sage/temp/lolita_4.local/85064/dir_2/pickle_jar/_class__sage_combinat_tableau_StandardTableaux_n__.sobj') * unpickle failure: load('/Users/anne/.sage/temp/lolita_4.local/85064/dir_2/pickle_jar/_class__sage_combinat_tableau_StandardTableaux_partition__.sobj') * unpickle failure: load('/Users/anne/.sage/temp/lolita_4.local/85064/dir_2/pickle_jar/_class__sage_combinat_tableau_Tableau_class__.sobj') * unpickle failure: load('/Users/anne/.sage/temp/lolita_4.local/85064/dir_2/pickle_jar/_class__sage_combinat_tableau_Tableaux_n__.sobj') doctest:1172: DeprecationWarning: This class is replaced by Matrix_modn_dense_float/Matrix_modn_dense_double. See http://trac.sagemath.org/4260 for details. Failed: _class__sage_combinat_crystals_affine_AffineCrystalFromClassicalAndPromotion_with_category_element_class__.sobj _class__sage_combinat_crystals_tensor_product_CrystalOfTableaux_with_category_element_class__.sobj _class__sage_combinat_crystals_tensor_product_TensorProductOfCrystalsWithGenerators_with_category__.sobj _class__sage_combinat_skew_tableau_SemistandardSkewTableaux_n__.sobj _class__sage_combinat_skew_tableau_SemistandardSkewTableaux_nmu__.sobj _class__sage_combinat_skew_tableau_SemistandardSkewTableaux_p__.sobj _class__sage_combinat_skew_tableau_SemistandardSkewTableaux_pmu__.sobj _class__sage_combinat_skew_tableau_StandardSkewTableaux_n__.sobj _class__sage_combinat_tableau_SemistandardTableaux_n__.sobj _class__sage_combinat_tableau_SemistandardTableaux_nmu__.sobj _class__sage_combinat_tableau_SemistandardTableaux_p__.sobj _class__sage_combinat_tableau_SemistandardTableaux_pmu__.sobj _class__sage_combinat_tableau_StandardTableaux_n__.sobj _class__sage_combinat_tableau_StandardTableaux_partition__.sobj _class__sage_combinat_tableau_Tableau_class__.sobj _class__sage_combinat_tableau_Tableaux_n__.sobj Successfully unpickled 571 objects. Failed to unpickle 16 objects. ********************************************************************** 1 items had failures: 1 of 7 in __main__.example_25 ***Test Failed*** 1 failures. For whitespace errors, see the file /Users/anne/.sage/tmp/lolita_4.local-73054/sage_object_85063.py [14.0 s]
I suppose this is due to your renaming of the classes. Also, I am not sure, but you might have to put in deprecation warnings if certain parent classes suddenly disappear. The new deprecation syntax is available here http://trac.sagemath.org/sage_trac/ticket/13109.
Best,
Anne
comment:23 in reply to: ↑ 22 Changed 8 years ago by
Hi again,
When building the docs using
sage -docbuild reference html
I got the following warning, which should most likely be fixed
/Applications/sage-5.3.beta0/local/lib/python2.7/site-packages/sage/combinat/tableau.py:docstring of sage.combinat.tableau.Tableau.lambda_katabolism:1: WARNING: Inline literal start-string without end-string.
Best,
Anne
comment:24 in reply to: ↑ 22 Changed 8 years ago by
Thanks Anne!
The pickling error confused me no end:) It seems that sage keeps pickles of old objects and then checks that new code is still able to unpickle the saved pickles. One way to fix this error would be be to make a new pickle jar which would remove the references to these renamed classes and presumably solve this problem. Another option would be to include deprecation warnings for all of the old class names -- I am not sure but would this lead to deprecation warnings during the unpickling meaning that these tests would still fail?
I am happy to deprecate all of the old class name if you like, although it does seem a little strange to deprecate classes that were never officially part of sage. Other the other hand, the patch has been around for a while so people may be using the old names in their own code, so it might be worth doing because of this.
I am happy to defer to your expertise as to what is the best course of action. Please advise.
The doc string error is also a little strange. It is caused by a T'_a in the doc string, but as this is part of a (raw) string I would have thought that sphinx would not have a problem with this... Anyway, I have fixed this by changing T'_a into S_a etc which is more readable anyway.
I won't upload a new patch until I hear back from about the best way to resolve the pickling issues.
Best, Andrew
comment:25 follow-up: ↓ 26 Changed 8 years ago by
Hi Anne,
I have discovered the register_unpickle_override function. Using this I have eliminated all but the following four unpickling errors:
- _classsage_combinat_crystals_affine_AffineCrystalFromClassicalAndPromotion_with_category_element_class.sobj
- _classsage_combinat_crystals_tensor_product_CrystalOfTableaux_with_category_element_class.sobj
- _classsage_combinat_crystals_tensor_product_TensorProductOfCrystalsWithGenerators_with_category.sobj
- _classsage_combinat_tableau_Tableau_class.sobj
These error are, I think, all due to the fact that the old Tableau_class has been replaced with the quite different Tableau class. My guess is that the only way to fix these unpicking errors is to update the pickle jar. Is this right? If so, then it probably is not worthwhile including all of the register_unpickle_override statements that I have just inserted. Let me know what you think.
I will upload a patch which fixes the docbuild problem that you mentioned, together with a few typos in the documentation and most of the unpickling problems, but I will hold off deprecation and the four problems above until I hear from you.
Best,
Andrew
comment:26 in reply to: ↑ 25 Changed 8 years ago by
Hi Andrew,
I sent some comments to you by e-mail.
Anne
comment:27 Changed 8 years ago by
Thanks for all of these Anne. I have updated the pickles and and added deprecations for all of the old tableaux class names. Perhaps I missed something obvious, but the later turned out to be quite painful as the deprecation scheme does not seem to support deprecation of internal classes -- there is a (one-sided) discussion about what I think goes wrong on sage-devel. I ended up writing dummy function wrappers for the deprecations, which came to about 140 lines of code because of the necessary doctests to keep sage --coverage happy. It would be nice if there was a better way...
Andrew
comment:28 Changed 8 years ago by
- Description modified (diff)
Apply trac_9265_tableaux_categories_am.patch
comment:29 Changed 8 years ago by
For the patchbot:
Apply trac_9265_tableaux_categories_am.patch
comment:30 follow-up: ↓ 31 Changed 8 years ago by
- Keywords tableaux combinatorics added
- Reviewers set to Andrew Mathas, Anne Schilling
comment:31 in reply to: ↑ 30 Changed 8 years ago by
This patch cleans up tableaux in combinatorics. Andrew took over Jason's patch and made all changes we discussed by e-mail. I ran all tests on the new machine in Washington combinat.math.washington.edu and all tests passed.
Positive review!
Anne
comment:32 Changed 8 years ago by
- Status changed from needs_review to positive_review
comment:33 in reply to: ↑ 18 Changed 8 years ago by
Replying to andrew.mathas:
As the patchbot was complaining, I just uploaded a new version of the patch which deletes all trailing white space. Probably this is unwise as previously I used to have my editor do this automatically but I found that it meant that I had to rebase my patch all of the time so this will probably cause havoc further down the queue?
What is the accepted practise here?
For the record: removing all trailing white spaces is indeed likely to produce conflicts. So I usually just make sure in my patch to not introduce new ones (sometimes, I edit the patch directly to remove those that I introduced accidently), and to remove those that are very close to the lines I change anyway.
Now in the case at hand, you currently kind of own the tableau file, since everybody knows that you are working hard on it, and that it is thus not safe playing with it. Then, the potential conflicts are with yourself, so you are free to take whichever course of action which is practical for you.
Thanks for your work!
Nicolas
comment:34 Changed 8 years ago by
- Milestone changed from sage-5.3 to sage-5.4
comment:35 Changed 8 years ago by
- Status changed from positive_review to needs_work
Please fix the commit message, it shouldn't refer to the [mq]
patch name.
comment:36 Changed 8 years ago by
- Status changed from needs_work to positive_review
Commit message fixed.
comment:37 Changed 7 years ago by
- Status changed from positive_review to needs_work
Doctest failure:
sage -t -force_lib devel/sage/sage/combinat/tableau.py ********************************************************************** File "/release/merger/sage-5.4.beta0/devel/sage-main/sage/combinat/tableau.py", line 4257: sage: sage.combinat.tableau.StandardTableaux_partition([2,1]) Expected: doctest:1: DeprecationWarning: this class is deprecated. Use StandardTableaux_size instead See http://trac.sagemath.org/9265 for details. Standard tableaux of shape [2, 1] Got: doctest:1: DeprecationWarning: this class is deprecated. Use StandardTableaux_shape instead See http://trac.sagemath.org/9265 for details. Standard tableaux of shape [2, 1] **********************************************************************
comment:38 Changed 7 years ago by
Sorry, I uploaded the wrong "new version" of the patch. This one should be OK.
All of the tests pasted for the patchbot (except for the pickles which need to be updated), so I am changing this back to positive review.
comment:39 Changed 7 years ago by
- Status changed from needs_work to positive_review
comment:40 Changed 7 years ago by
- Merged in set to sage-5.4.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
comment:41 Changed 7 years ago by
- Dependencies set to #5457
comment:42 follow-up: ↓ 43 Changed 7 years ago by
This patch badly abuses assert
and AssertionError
. assert
should not be used for control flow. An assert checks something which should always be true, a failed assertion is always a bug in the program.
For example:
sage: Tableau([[1],[2,3]]) Traceback (most recent call last): ... AssertionError: A tableau must be a list of lists of weakly decreasing length.
This is a simple user mistake, for which assert
is not right.
I think this must be fixed.
comment:43 in reply to: ↑ 42 ; follow-ups: ↓ 44 ↓ 45 ↓ 49 Changed 7 years ago by
Hi Jeroen,
Replying to jdemeyer:
This patch badly abuses
assert
andAssertionError
.assert
should not be used for control flow. An assert checks something which should always be true, a failed assertion is always a bug in the program.For example:
sage: Tableau([[1],[2,3]]) Traceback (most recent call last): ... AssertionError: A tableau must be a list of lists of weakly decreasing length.This is a simple user mistake, for which
assert
is not right.I think this must be fixed.
There is no control flow involved. It's quite a common public constructor, but speed matters because it's used a lot at a low level in combinatorics calculations. The error message is nice and explicit. Altogether, given the discussion on sage-devel, do we agree that it's ok as such?
Cheers,
Nicolas
comment:44 in reply to: ↑ 43 ; follow-up: ↓ 46 Changed 7 years ago by
Replying to nthiery:
There is no control flow involved.
I disagree.
try: ... except AssertionError: ...
is certainly control flow.
It's quite a common public constructor, but speed matters because it's used a lot at a low level in combinatorics calculations.
Are these constructors really that speed-critical? Of the 3 patches (#9265, #8899, #5457), this one is certainly the worst offender.
comment:45 in reply to: ↑ 43 ; follow-up: ↓ 47 Changed 7 years ago by
Replying to nthiery:
speed matters because it's used a lot at a low level in combinatorics calculations.
If speed matters that much, you could probably get a lot more speedup by using Cython as opposed to less argument checking.
comment:46 in reply to: ↑ 44 ; follow-up: ↓ 48 Changed 7 years ago by
Replying to jdemeyer:
There is no control flow involved.
I disagree.
try: ... except AssertionError: ...is certainly control flow.
Ouch, that piece? Yes certainly, it's bad and should be fixed, either by using a ValueError?, or better by avoiding to feed Tableau with potential garbage.
There is a lot of action going on currently around Tableaux so, to keep things running, I recommend doing that in a later ticket to not delay other stuff.
comment:47 in reply to: ↑ 45 Changed 7 years ago by
Replying to jdemeyer:
speed matters because it's used a lot at a low level in combinatorics calculations.
If speed matters that much, you could probably get a lot more speedup by using Cython
Certainly, that's the planned next step in the cleanup of tableaux :-) And then having checks that can be disabled will really become relevant.
comment:48 in reply to: ↑ 46 Changed 7 years ago by
Replying to nthiery:
There is a lot of action going on currently around Tableaux so, to keep things running, I recommend doing that in a later ticket to not delay other stuff.
Good for me, it's just something to keep in mind.
comment:49 in reply to: ↑ 43 Changed 7 years ago by
Replying to nthiery:
It's quite a common public constructor, but speed matters because it's used a lot at a low level in combinatorics calculations.
Then I think the best solution is adding a "check" argument to the constructor defaulting to True and doing the expensive checks only when check
is True.
comment:50 Changed 7 years ago by
Jason seemed to like these assertions checks, but I agree that are "wrong" -- in fact, they caused me some grief in #13074. I doubt that there will be speed implications in modifying them.
Unfortunately, it may take me a while to fix this as I'm at a conference this week and I don't have a good internet connection.
Andrew
comment:51 Changed 7 years ago by
...talk written, patch patched.
comment:52 follow-up: ↓ 53 Changed 7 years ago by
Wow, thanks a lot! Anne or Jason, could any of you do the review please?
comment:53 in reply to: ↑ 52 ; follow-up: ↓ 54 Changed 7 years ago by
The patch on the sage-combinat queue looks good to me (which I assume is the same as here).
Anne
comment:54 in reply to: ↑ 53 Changed 7 years ago by
Replying to aschilling:
The patch on the sage-combinat queue looks good to me (which I assume is the same as here).
Anne
Thanks Anne! Yes, it's the same patch as the one on the queue. Andrew
comment:55 Changed 7 years ago by
I'm a bit scared by this pickle jar update now that Andrew Mathas admitted on #13072 that he doesn't know how to update the pickle jar.
comment:56 Changed 7 years ago by
Hi Jeroen,
I did this pickle jar update following detailed instructions that Anne Schilling sent me. What I asked for on #13072 was for proper documentation on how to update pickles because I thought that the pickle may not have been applied properly (in fact, everything is fine because I was looking at version 5.3 whereas the updated pickles are in 5.4). As far as I am aware, there is no documentation on how to update the pickle jar, only instructions passed on hand-to-mouth, which IS a problem. For example, this doesn't seem to be covered in the developer guide.
Andrew
comment:57 Changed 7 years ago by
You are not supposed to update the pickle jar, it is only here to ensure backward compatibility. If at all possible, you should be using the register_unpickle_override
to work around the change and unpickle into the new class.
comment:58 Changed 7 years ago by
I have just uploaded the patch trac_9265--tableaux_categories_pickles-am.patch which adds unpickle overrides for most of the old classes that are being deprecated. This fixes all but four of the unpickle problems, however, it does not fix unpickling for Tableau_class. I think that because Tableau_class does not unpickle the following four pickles still fail:
sage_combinat_crystals_affine_AffineCrystalFromClassicalAndPromotion_with_category_element_class
sage_combinat_crystals_tensor_product_CrystalOfTableaux_with_category_element_class
sage_combinat_crystals_tensor_product_TensorProductOfCrystalsWithGenerators_with_category
sage_combinat_tableau_Tableau_class
I have tried to fix the unpickling of Tableau_class using
register_unpickle_override('sage.combinat.tableau', 'Tableau_class', Tableau)
but this does not work. My guess is that it is not possible to unpickle the deprecated Tableau_class objects using the new Tableau class objects because the underlying classes are too different.
If some one can see how to do this please let me know.
comment:59 Changed 7 years ago by
- Merged in sage-5.4.beta0 deleted
- Milestone changed from sage-5.4 to sage-5.5
- Resolution fixed deleted
- Status changed from closed to new
Let's postpone this to sage-5.5 such that the pickling issues can quietly be ironed out.
comment:60 Changed 7 years ago by
- Description modified (diff)
comment:61 Changed 7 years ago by
All pickle problems resolved.
comment:62 Changed 7 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:63 Changed 7 years ago by
- Description modified (diff)
comment:64 follow-up: ↓ 66 Changed 7 years ago by
Thanks andrew for going the extra mile for backward compatibility!
comment:65 Changed 7 years ago by
Hi Anne,
Would you mind reviewing the latest update to #9265 so that Jeroen can put in back in the merge queue. It is just a matter of testing that
sage -t devel/sage-sf/sage/structure/sage_object.pyx
works. The new patch trac_9265--tableaux_categories_pickles-am.patch is also in the sage-combinat queue and to test it you should use 5.3 as 5.4 has the wrong pickle_jar at present.
Cheers,
Andrew
comment:66 in reply to: ↑ 64 ; follow-ups: ↓ 67 ↓ 69 Changed 7 years ago by
Replying to nthiery:
Thanks andrew for going the extra mile for backward compatibility!
Well, I didn't have a lot of choice:) It's a pity (but understandable) that Jeroen bumped the patch out of the 5.4 release an hour before I uploaded the fix as I guess this will play havoc with the sage-combinat queue. I am sure sure how we can guard for different versions of 5.4.? in the queue as "old" pre-releases will have the patch but one current ones won't.
comment:67 in reply to: ↑ 66 ; follow-up: ↓ 70 Changed 7 years ago by
Hi Andrew,
I ran all tests and looked at the new patch. It looks fine. All tests pass with sage-5.3, except, but this seems unrelated to your patch and more related to a file by Jeroen Demeyer. Hence I am setting a positive review.
Thanks!
Anne
sage -t sage/tests/cmdline.py sage -t "devel/sage-combinat/sage/tests/cmdline.py" File "/Applications/sage-5.3/devel/sage-combinat/sage/tests/cmdline.py", line 99:
sage: err
Expected:
Got:
'---------------------------------------------------------------------------\nAttributeError Traceback (most recent call last)\n\n/Applications/sage-5.3/devel/sage-combinat/<ipython console> in <module>()\n\n/Applications/sage-5.3/local/lib/python/site-packages/sage/misc/preparser.pyc in load(filename, globals, attach)\n 1657 else:\n 1658 # Preparse in memory only for speed.\n-> 1659 exec(preparse_file(open(fpath).read()) + "
n", globals)\n 1660 elif fpath.endswith(\'.spyx\') or fpath.endswith(\'.pyx\'):\n 1661 import interpreter\n\n/Applications/sage-5.3/devel/sage-combinat/<string> in <module>()\n\nAttributeError: Latex instance has no attribute \'add_to_mathjax_avoid_list\'\n'
File "/Applications/sage-5.3/devel/sage-combinat/sage/tests/cmdline.py", line 109:
sage: err
Expected:
Got:
'---------------------------------------------------------------------------\nAttributeError Traceback (most recent call last)\n\n/Applications/sage-5.3/devel/sage-combinat/<ipython console> in <module>()\n\n/Applications/sage-5.3/local/lib/python/site-packages/sage/misc/preparser.pyc in load(filename, globals, attach)\n 1657 else:\n 1658 # Preparse in memory only for speed.\n-> 1659 exec(preparse_file(open(fpath).read()) + "
n", globals)\n 1660 elif fpath.endswith(\'.spyx\') or fpath.endswith(\'.pyx\'):\n 1661 import interpreter\n\n/Applications/sage-5.3/devel/sage-combinat/<string> in <module>()\n\nAttributeError: Latex instance has no attribute \'add_to_mathjax_avoid_list\'\n'
File "/Applications/sage-5.3/devel/sage-combinat/sage/tests/cmdline.py", line 119:
sage: err
Expected:
Got:
'---------------------------------------------------------------------------\nAttributeError Traceback (most recent call last)\n\n/Applications/sage-5.3/devel/sage-combinat/<ipython console> in <module>()\n\n/Applications/sage-5.3/local/lib/python/site-packages/sage/misc/preparser.pyc in load(filename, globals, attach)\n 1657 else:\n 1658 # Preparse in memory only for speed.\n-> 1659 exec(preparse_file(open(fpath).read()) + "
n", globals)\n 1660 elif fpath.endswith(\'.spyx\') or fpath.endswith(\'.pyx\'):\n 1661 import interpreter\n\n/Applications/sage-5.3/devel/sage-combinat/<string> in <module>()\n\nAttributeError: Latex instance has no attribute \'add_to_mathjax_avoid_list\'\n'
1 items had failures:
3 of 187 in main.example_1
*Test Failed* 3 failures. For whitespace errors, see the file /Users/anne/.sagetmp/cmdline_49442.py
[27.8 s]
comment:68 Changed 7 years ago by
- Status changed from needs_review to positive_review
comment:69 in reply to: ↑ 66 Changed 7 years ago by
Replying to andrew.mathas:
It's a pity (but understandable) that Jeroen bumped the patch out of the 5.4 release an hour before I uploaded the fix as I guess this will play havoc with the sage-combinat queue. I am sure sure how we can guard for different versions of 5.4.? in the queue as "old" pre-releases will have the patch but one current ones won't.
Well, once 5.4 will be out, we will just state that we don't support anymore the beta/rc of 5.4. Only a few of us are using them anyway, so that's anoying but not critical.
comment:70 in reply to: ↑ 67 Changed 7 years ago by
Thanks Anne
comment:71 Changed 7 years ago by
- Merged in set to sage-5.5.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
Milestone sage-4.4.5 deleted