Remove `CombinatorialClass` from sage.combinat.tableau

jbandlow commented 14 years ago

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 commented 14 years ago

Milestone sage-4.4.5 deleted

jbandlow commented 13 years ago

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 commented 13 years ago

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 commented 13 years ago

Description changed:

--- 
+++ 
@@ -1 +1,3 @@
 The `CombinatorialClass` class is being deprecated.  See [Sage Combinat Roadmap](http://trac.sagemath.org/sage_trac/wiki/SageCombinatRoadMap) for more information.  This ticket will handle removing this class from sage.combinat.tableau.  See also some discussion of this on [this thread](http://groups.google.com/group/sage-combinat-devel/browse_thread/thread/1819418007f5157).
+
+**Apply:** [attachment: trac_9265_tableaux_categories_jb.patch](https://github.com/sagemath/sage/files/ticket9265/trac_9265_tableaux_categories_jb.patch.gz)

jbandlow commented 13 years ago

Dependencies: #10603, #11314

jbandlow commented 13 years ago

Author: Jason Bandlow

jbandlow commented 13 years ago

comment:4

Updated the patch to reflect the discussion here.

dimpase commented 12 years ago

comment:5

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

AndrewMathas commented 12 years ago

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 commented 12 years ago

comment:8

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

AndrewMathas commented 12 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,3 @@
 The `CombinatorialClass` class is being deprecated.  See [Sage Combinat Roadmap](http://trac.sagemath.org/sage_trac/wiki/SageCombinatRoadMap) for more information.  This ticket will handle removing this class from sage.combinat.tableau.  See also some discussion of this on [this thread](http://groups.google.com/group/sage-combinat-devel/browse_thread/thread/1819418007f5157).

-**Apply:** [attachment: trac_9265_tableaux_categories_jb.patch](https://github.com/sagemath/sage/files/ticket9265/trac_9265_tableaux_categories_jb.patch.gz)
+**Apply:** [attachment: trac_9265_tableaux_categories_am.patch](https://github.com/sagemath/sage-prod/files/10649609/trac_9265_tableaux_categories_am.patch.gz)

AndrewMathas commented 12 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,5 @@
 The `CombinatorialClass` class is being deprecated.  See [Sage Combinat Roadmap](http://trac.sagemath.org/sage_trac/wiki/SageCombinatRoadMap) for more information.  This ticket will handle removing this class from sage.combinat.tableau.  See also some discussion of this on [this thread](http://groups.google.com/group/sage-combinat-devel/browse_thread/thread/1819418007f5157).

-**Apply:** [attachment: trac_9265_tableaux_categories_am.patch](https://github.com/sagemath/sage-prod/files/10649609/trac_9265_tableaux_categories_am.patch.gz)
+
+
+Apply: trac_9265_tableaux_categories_am.patch

saliola commented 12 years ago

comment:10

For the patchbot:

Apply: trac_9265_tableaux_categories_am.patch

AndrewMathas commented 12 years ago

Changed dependencies from #10603, #11314 to none

AndrewMathas commented 12 years ago

comment:11

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

AndrewMathas commented 12 years ago

Description changed:

--- 
+++ 
@@ -1,5 +1,4 @@
 The `CombinatorialClass` class is being deprecated.  See [Sage Combinat Roadmap](http://trac.sagemath.org/sage_trac/wiki/SageCombinatRoadMap) for more information.  This ticket will handle removing this class from sage.combinat.tableau.  See also some discussion of this on [this thread](http://groups.google.com/group/sage-combinat-devel/browse_thread/thread/1819418007f5157).

-
 Apply: trac_9265_tableaux_categories_am.patch

AndrewMathas commented 12 years ago

Description changed:

--- 
+++ 
@@ -2,3 +2,5 @@

 Apply: trac_9265_tableaux_categories_am.patch
+
+**Apply:** [attachment: trac_9265_tableaux_categories_am.patch](https://github.com/sagemath/sage-prod/files/10649609/trac_9265_tableaux_categories_am.patch.gz)

AndrewMathas commented 12 years ago

Description changed:

--- 
+++ 
@@ -1,6 +1,5 @@
 The `CombinatorialClass` class is being deprecated.  See [Sage Combinat Roadmap](http://trac.sagemath.org/sage_trac/wiki/SageCombinatRoadMap) for more information.  This ticket will handle removing this class from sage.combinat.tableau.  See also some discussion of this on [this thread](http://groups.google.com/group/sage-combinat-devel/browse_thread/thread/1819418007f5157).

-
-Apply: trac_9265_tableaux_categories_am.patch
+ 

 **Apply:** [attachment: trac_9265_tableaux_categories_am.patch](https://github.com/sagemath/sage-prod/files/10649609/trac_9265_tableaux_categories_am.patch.gz)

AndrewMathas commented 12 years ago

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 commented 12 years ago

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 commented 12 years ago

comment:17

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

AndrewMathas commented 12 years ago

comment:18

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?

anneschilling commented 12 years ago

comment:19

Replying to @AndrewAtLarge:

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

anneschilling commented 12 years ago

comment:20

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

AndrewMathas commented 12 years ago

comment:21

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

anneschilling commented 12 years ago

comment:22

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 https://github.com/sagemath/sage-prod/issues/13109.

Best,

Anne

anneschilling commented 12 years ago

comment:23

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

AndrewMathas commented 12 years ago

comment:24

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

AndrewMathas commented 12 years ago

comment:25

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
_class__sage_combinat_crystals_tensor_product_CrystalOfTableaux_with_category_element_class__.sobj
_class__sage_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

anneschilling commented 12 years ago

comment:26

Hi Andrew,

I sent some comments to you by e-mail.

Anne

AndrewMathas commented 12 years ago

comment:27

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

AndrewMathas commented 12 years ago

Description changed:

--- 
+++ 
@@ -1,5 +1,5 @@
 The `CombinatorialClass` class is being deprecated.  See [Sage Combinat Roadmap](http://trac.sagemath.org/sage_trac/wiki/SageCombinatRoadMap) for more information.  This ticket will handle removing this class from sage.combinat.tableau.  See also some discussion of this on [this thread](http://groups.google.com/group/sage-combinat-devel/browse_thread/thread/1819418007f5157).

- 
+**Apply:** [attachment: trac_9265_tableaux_categories_am.patch](https://github.com/sagemath/sage-prod/files/10649609/trac_9265_tableaux_categories_am.patch.gz)

-**Apply:** [attachment: trac_9265_tableaux_categories_am.patch](https://github.com/sagemath/sage-prod/files/10649609/trac_9265_tableaux_categories_am.patch.gz)
+**Update pickles:** [attachment: pickle_jar.tar.bz2](https://github.com/sagemath/sage/files/ticket9265/pickle_jar.tar.bz2.gz)

AndrewMathas commented 12 years ago

comment:28

Apply trac_9265_tableaux_categories_am.patch

AndrewMathas commented 12 years ago

comment:29

For the patchbot:

Apply trac_9265_tableaux_categories_am.patch

anneschilling commented 12 years ago

Reviewer: Andrew Mathas, Anne Schilling

anneschilling commented 12 years ago

Changed keywords from none to tableaux

anneschilling commented 12 years ago

Changed author from Jason Bandlow to Jason Bandlow, Andrew Mathas

anneschilling commented 12 years ago

comment:31

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

nthiery commented 12 years ago

comment:33

Replying to @AndrewAtLarge:

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

jdemeyer commented 12 years ago

comment:35

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

AndrewMathas commented 12 years ago

comment:36

Commit message fixed.

jdemeyer commented 12 years ago

comment:37

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]
**********************************************************************

AndrewMathas commented 12 years ago

comment:38

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.

jdemeyer commented 12 years ago

Merged: sage-5.4.beta0

jdemeyer commented 12 years ago

Dependencies: #5457

jdemeyer commented 12 years ago

comment:42

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.

nthiery commented 12 years ago

comment:43

Hi Jeroen,

Replying to @jdemeyer:

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

jdemeyer commented 12 years ago

comment:44

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.

jdemeyer commented 12 years ago

comment:45

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.

nthiery commented 12 years ago

comment:46

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.

sagemath / sage

Remove `CombinatorialClass` from sage.combinat.tableau #9265