Opened 12 years ago

Closed 10 years ago

# Remove CombinatorialClass from sage.combinat.tableau

Reported by: Owned by: Jason Bandlow Sage Combinat CC user major sage-5.5 combinatorics tableaux, combinatorics sage-5.5.beta0 Jason Bandlow, Andrew Mathas Andrew Mathas, Anne Schilling N/A #5457

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.

(and don't update the pickle jar!)

### comment:1 Changed 12 years ago by William Stein

Milestone: sage-4.4.5 → sage-4.5

Milestone sage-4.4.5 deleted

### comment:2 Changed 12 years ago by Jason Bandlow

There is now a patch on the sage-combinat queue, which can be viewed here:

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 11 years ago by Jason Bandlow

Authors: → Jason Bandlow → #10603, #11314 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 11 years ago by Jason Bandlow

Updated the patch to reflect the discussion here.

### comment:5 Changed 10 years ago by Dima Pasechnik

Status: new → needs_review

needs a (trivial, hopefully) rebase for Sage 5.2.rc0

### comment:6 Changed 10 years ago by Dima Pasechnik

Status: needs_review → needs_work

### comment:7 Changed 10 years ago by Andrew Mathas

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 10 years ago by Andrew Mathas

Description: modified (diff) needs_work → needs_review

Patch rebased so that it now applies cleanly to the top of sage 5.2.

### comment:9 Changed 10 years ago by Andrew Mathas

Description: modified (diff)

### comment:10 Changed 10 years ago by Franco Saliola

For the patchbot:

Apply: trac_9265_tableaux_categories_am.patch

### comment:11 Changed 10 years ago by Andrew Mathas

Dependencies: #10603, #11314 modified (diff)

Removed dependencies #10603 and #11314 are they were merged in 4.7 and 5.0, respectively.

### comment:12 Changed 10 years ago by Andrew Mathas

Description: modified (diff)

### comment:13 Changed 10 years ago by Andrew Mathas

Description: modified (diff)

### comment:14 Changed 10 years ago by Andrew Mathas

Last edited 10 years ago by Andrew Mathas (previous) (diff)

### comment:15 follow-up:  16 Changed 10 years ago by 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

Last edited 10 years ago by Andrew Mathas (previous) (diff)

### comment:16 in reply to:  15 Changed 10 years ago by Anne Schilling

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 10 years ago by Andrew Mathas

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???

Last edited 10 years ago by Andrew Mathas (previous) (diff)

### comment:18 follow-ups:  19  33 Changed 10 years ago by 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?

### comment:19 in reply to:  18 ; follow-up:  20 Changed 10 years ago by Anne Schilling

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 10 years ago by Anne Schilling

Dear Andrew,

Here are a couple of comments on the ticket:

• 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 10 years ago by Andrew Mathas

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

Last edited 10 years ago by Andrew Mathas (previous) (diff)

### comment:22 in reply to:  21 ; follow-ups:  23  24 Changed 10 years ago by Anne Schilling

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:
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 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 10 years ago by Anne Schilling

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 10 years ago by Andrew Mathas

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

Last edited 10 years ago by Andrew Mathas (previous) (diff)

### comment:25 follow-up:  26 Changed 10 years ago by Andrew Mathas

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 10 years ago by Anne Schilling

Hi Andrew,

I sent some comments to you by e-mail.

Anne

### comment:27 Changed 10 years ago by Andrew Mathas

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

Last edited 10 years ago by Andrew Mathas (previous) (diff)

### comment:28 Changed 10 years ago by Andrew Mathas

Description: modified (diff)

Apply trac_9265_tableaux_categories_am.patch

Last edited 10 years ago by Andrew Mathas (previous) (diff)

### comment:29 Changed 10 years ago by Andrew Mathas

For the patchbot:

Apply trac_9265_tableaux_categories_am.patch

### comment:30 follow-up:  31 Changed 10 years ago by Anne Schilling

Authors: Jason Bandlow → Jason Bandlow, Andrew Mathas tableaux combinatorics added → Andrew Mathas, Anne Schilling

### comment:31 in reply to:  30 Changed 10 years ago by Anne Schilling

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 10 years ago by Andrew Mathas

Status: needs_review → positive_review

### comment:33 in reply to:  18 Changed 10 years ago by Nicolas M. Thiéry

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.

Nicolas

### comment:34 Changed 10 years ago by Jeroen Demeyer

Milestone: sage-5.3 → sage-5.4

### comment:35 Changed 10 years ago by Jeroen Demeyer

Status: positive_review → needs_work

Please fix the commit message, it shouldn't refer to the [mq] patch name.

### comment:36 Changed 10 years ago by Andrew Mathas

Status: needs_work → positive_review

Commit message fixed.

### comment:37 Changed 10 years ago by Jeroen Demeyer

Status: positive_review → 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 10 years ago by Andrew Mathas

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.

Last edited 10 years ago by Andrew Mathas (previous) (diff)

### comment:39 Changed 10 years ago by Andrew Mathas

Status: needs_work → positive_review

### comment:40 Changed 10 years ago by Jeroen Demeyer

Merged in: → sage-5.4.beta0 → fixed positive_review → closed

### comment:41 Changed 10 years ago by Jeroen Demeyer

Dependencies: → #5457

### comment:42 follow-up:  43 Changed 10 years ago by Jeroen Demeyer

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 10 years ago by Nicolas M. Thiéry

Hi Jeroen,

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.

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 10 years ago by Jeroen Demeyer

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 10 years ago by Jeroen Demeyer

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 10 years ago by Nicolas M. Thiéry

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 10 years ago by Nicolas M. Thiéry

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 10 years ago by Jeroen Demeyer

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 10 years ago by Jeroen Demeyer

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 10 years ago by Andrew Mathas

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

### Changed 10 years ago by Andrew Mathas

Removing assert statements

### comment:51 Changed 10 years ago by Andrew Mathas

...talk written, patch patched.

### comment:52 follow-up:  53 Changed 10 years ago by Jeroen Demeyer

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 10 years ago by Anne Schilling

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 10 years ago by Andrew Mathas

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 10 years ago by Jeroen Demeyer

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 10 years ago by Andrew Mathas

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

Last edited 10 years ago by Andrew Mathas (previous) (diff)

### comment:57 Changed 10 years ago by Volker Braun

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 10 years ago by Andrew Mathas

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.

Last edited 10 years ago by Andrew Mathas (previous) (diff)

### comment:59 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.4.beta0 sage-5.4 → sage-5.5 fixed closed → new

Let's postpone this to sage-5.5 such that the pickling issues can quietly be ironed out.

### comment:60 Changed 10 years ago by Andrew Mathas

Description: modified (diff)

### comment:61 Changed 10 years ago by Andrew Mathas

All pickle problems resolved.

### comment:62 Changed 10 years ago by Andrew Mathas

Description: modified (diff) new → needs_review

### comment:63 Changed 10 years ago by Andrew Mathas

Description: modified (diff)

### Changed 10 years ago by Andrew Mathas

Fixing a typo in a comment

### comment:64 follow-up:  66 Changed 10 years ago by Nicolas M. Thiéry

Thanks andrew for going the extra mile for backward compatibility!

### comment:65 Changed 10 years ago by Andrew Mathas

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 10 years ago by Andrew Mathas

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 10 years ago by Anne Schilling

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'

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 10 years ago by Anne Schilling

Status: needs_review → positive_review

### comment:69 in reply to:  66 Changed 10 years ago by Nicolas M. Thiéry

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 10 years ago by Andrew Mathas

Thanks Anne

Last edited 10 years ago by Andrew Mathas (previous) (diff)

### comment:71 Changed 10 years ago by Jeroen Demeyer

Merged in: → sage-5.5.beta0 → fixed positive_review → closed
Note: See TracTickets for help on using tickets.