Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove CombinatorialClass from sage.combinat.tableau #9265

Closed
jbandlow opened this issue Jun 18, 2010 · 77 comments
Closed

Remove CombinatorialClass from sage.combinat.tableau #9265

jbandlow opened this issue Jun 18, 2010 · 77 comments

Comments

@jbandlow
Copy link

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: attachment: trac_9265_tableaux_categories_am.patch

and then

attachment: trac_9265--tableaux_categories_pickles-am.patch

(and don't update the pickle jar!)

Depends on #5457

Component: combinatorics

Keywords: tableaux

Author: Jason Bandlow, Andrew Mathas

Reviewer: Andrew Mathas, Anne Schilling

Merged: sage-5.5.beta0

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

@williamstein
Copy link
Contributor

Milestone sage-4.4.5 deleted

@jbandlow
Copy link
Author

comment:2

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!

@jbandlow
Copy link
Author

comment:3

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!

@jbandlow

This comment has been minimized.

@jbandlow
Copy link
Author

Dependencies: #10603, #11314

@jbandlow
Copy link
Author

Author: Jason Bandlow

@jbandlow
Copy link
Author

comment:4

Updated the patch to reflect the discussion here.

@dimpase
Copy link
Member

dimpase commented Jul 24, 2012

comment:5

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

@AndrewMathas
Copy link
Member

comment:7

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

@AndrewMathas
Copy link
Member

comment:8

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

@AndrewMathas

This comment has been minimized.

@AndrewMathas

This comment has been minimized.

@saliola
Copy link

saliola commented Aug 1, 2012

comment:10

For the patchbot:

Apply: trac_9265_tableaux_categories_am.patch

@AndrewMathas
Copy link
Member

Changed dependencies from #10603, #11314 to none

@AndrewMathas
Copy link
Member

comment:11

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

@AndrewMathas

This comment has been minimized.

@AndrewMathas

This comment has been minimized.

@AndrewMathas

This comment has been minimized.

@AndrewMathas
Copy link
Member

comment:15

It looks like the colon after the "Apply" above is confusing the patchbot. So let's try:

Apply trac_9265_tableaux_categories_am.patch

@anneschilling
Copy link

comment:16

Replying to @AndrewAtLarge:

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

@AndrewMathas
Copy link
Member

comment:56

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

@vbraun
Copy link
Member

vbraun commented Oct 15, 2012

comment:57

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.

@AndrewMathas
Copy link
Member

comment:58

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.

@jdemeyer
Copy link

Changed merged from sage-5.4.beta0 to none

@jdemeyer
Copy link

comment:59

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

@jdemeyer jdemeyer modified the milestones: sage-5.4, sage-5.5 Oct 17, 2012
@jdemeyer jdemeyer reopened this Oct 17, 2012
@AndrewMathas

This comment has been minimized.

@AndrewMathas
Copy link
Member

comment:61

All pickle problems resolved.

@AndrewMathas

This comment has been minimized.

@AndrewMathas

This comment has been minimized.

@AndrewMathas
Copy link
Member

Attachment: trac_9265--tableaux_categories_pickles-am.patch.gz

Fixing a typo in a comment

@nthiery
Copy link
Contributor

nthiery commented Oct 17, 2012

comment:64

Thanks andrew for going the extra mile for backward compatibility!

@AndrewMathas
Copy link
Member

comment:65

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

@AndrewMathas
Copy link
Member

comment:66

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.

@anneschilling
Copy link

comment:67

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/ in ()\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/ in ()\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/ in ()\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/ in ()\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/ in ()\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/ in ()\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/.sage//tmp/cmdline_49442.py
[27.8 s]

@nthiery
Copy link
Contributor

nthiery commented Oct 19, 2012

comment:69

Replying to @AndrewAtLarge:

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.

@AndrewMathas
Copy link
Member

comment:70

Thanks Anne

@jdemeyer
Copy link

Merged: sage-5.5.beta0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants