Problems with contains for Tableau, TableauTuples and PartitionTuples

[AndrewMathas]

The following three commands all lead to errors:

sage: 1 in Tableaux()
sage: 1 in TableauTuples()
sage: 1 in PartitionTuples()

In all cases, sage reports

TypeError: 'sage.rings.integer.Integer' object is not iterable

The place where sage is failing is in CombinatorialObject.init where we have:

        if isinstance(l, list):
            self._list = l
        else:
            self._list = list(l)

The error arises because l==1 is not a list. The discussion on sage combinat concluded that this was more a bug in the __contains__ methods of these classes rather than in CombinatorialObject.init -- although, I do think that CombinatorialObject should trap this error and print a more informative error message.

Anyway, contains should never give rise to an error, so the contains methods of these classes need to be fixed.

Depends on #13605

CC: @AndrewAtLarge

Component: combinatorics

Keywords: days45

Author: Travis Scrimshaw, Andrew Mathas

Reviewer: Andrew Mathas, Travis Scrimshaw

Merged: sage-5.10.beta2

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

[AndrewMathas]

Changed keywords from sage45 to days45

[tscrim]

Author: Travis Scrimshaw

[tscrim]

Dependencies: #13605

[tscrim]

comment:2

I've gone through and also caught all TypeError exceptions in the __contains__() methods. At the end of the day, I'd rather this to not try to create the element class, but this would likely require some major refactoring at best. Actually, in an ideal world I would like anytime a call to in throws an error, it returns False...I think I've probably said more than enough. Anyways, this patch fixes the problem at hand and is ready for review.

Best,

Travis

[AndrewMathas]

Reviewer: Andrew Mathas

[AndrewMathas]

comment:4

I will upload a "review" patch shortly which tests containment without creating an object in the element class.

[tscrim]

comment:5

Hey Andrew,

Just wondering what the status of the review patch is.

Thanks,

Travis

[AndrewMathas]

comment:6

Hi Travis,

I had it almost finished -- one class left to do -- and then teaching started and some other urgent projects got in the way. Will try and finalise soon, although it is going to be more of a replacement patch than a review patch I'm afraid...

Andrew

[tscrim]

comment:7

Hey Andrew,

No worries on both accounts, I completely understand and there's not any rush on this. Hope everything is going well.

Best,

Travis

[AndrewMathas]

Changed reviewer from Andrew Mathas to Andrew Mathas, Travis Scrimshaw

[AndrewMathas]

Changed author from Travis Scrimshaw to Travis Scrimshaw, Andrew Mathas

[AndrewMathas]

comment:8

Hi Travis,

I finally got back to this. I looked more closely at the changes in timing before and after the patch and I think that they are not in fact due to element creation but instead are caused by the lazy imports. For example, the biggest slowdown (from 0s to 6.51s) occurs with the lines

G = SymmetricGroup(8)
g = G([(1,2,3,4,5),(6,7,8)])

I have just pushed a slight modification to the patch to the combinat queue which changes the sanity checking in Partition.init. I ran a few tests and the new code is slightly faster than the old code.

Finally, as my first review patch was more 4 times bigger than your patch I have made us both authors and reviewers.

If you are happy with it then let's make this a positive review.

Andrew

[tscrim]

comment:9

Thanks for checking on the timings Andrew (I ended up forgetting about looking deeper into it, sorry). I've uploaded the patch from the queue. Looks good to me.

Thanks,

Travis

[jdemeyer]

comment:10

Why the change from

except StandardError:

to

except:

[tscrim]

comment:11

Sorry missed that one.

[jdemeyer]

comment:13

sage -t --long devel/sage/sage/combinat/combinat.py
**********************************************************************
File "devel/sage/sage/combinat/combinat.py", line 1365, in sage.combinat.combinat.CombinatorialClass.__call__
Failed example:
    p5([2,1])
Expected:
    Traceback (most recent call last):
    ...
    ValueError: [2, 1] is not in Partitions of the integer 5
Got:
    <BLANKLINE>
    Traceback (most recent call last):
      File "/mazur/release/merger/sage-5.10.beta2/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 454, in _run
        self.execute(example, compiled, test.globs)
      File "/mazur/release/merger/sage-5.10.beta2/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 813, in execute
        exec compiled in globs
      File "<doctest sage.combinat.combinat.CombinatorialClass.__call__[5]>", line 1, in <module>
        p5([Integer(2),Integer(1)])
      File "parent.pyx", line 930, in sage.structure.parent.Parent.__call__ (sage/structure/parent.c:8064)
      File "coerce_maps.pyx", line 82, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (sage/structure/coerce_maps.c:3815)
      File "coerce_maps.pyx", line 77, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (sage/structure/coerce_maps.c:3717)
      File "/mazur/release/merger/sage-5.10.beta2/local/lib/python2.7/site-packages/sage/combinat/partition.py", line 4043, in _element_constructor_
        raise ValueError('%s is not an element of %s'%(lst, self))
    ValueError: [2, 1] is not an element of Partitions of the integer 5
**********************************************************************

sage -t --long devel/sage/sage/combinat/sf/sf.py
**********************************************************************
File "devel/sage/sage/combinat/sf/sf.py", line 129, in sage.combinat.sf.sf.SymmetricFunctions
Failed example:
    p['something']
Expected:
    Traceback (most recent call last):
    ...
    ValueError: ['s', 'o', 'm', 'e', 't', 'h', 'i', 'n', 'g'] is not a valid partition
Got:
    <BLANKLINE>
    Traceback (most recent call last):
      File "/mazur/release/merger/sage-5.10.beta2/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 454, in _run
        self.execute(example, compiled, test.globs)
      File "/mazur/release/merger/sage-5.10.beta2/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 813, in execute
        exec compiled in globs
      File "<doctest sage.combinat.sf.sf.SymmetricFunctions[13]>", line 1, in <module>
        p['something']
      File "/mazur/release/merger/sage-5.10.beta2/local/lib/python2.7/site-packages/sage/combinat/sf/sfa.py", line 845, in __getitem__
        c = C(list(c))
      File "parent.pyx", line 930, in sage.structure.parent.Parent.__call__ (sage/structure/parent.c:8064)
      File "coerce_maps.pyx", line 82, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (sage/structure/coerce_maps.c:3815)
      File "coerce_maps.pyx", line 77, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (sage/structure/coerce_maps.c:3717)
      File "/mazur/release/merger/sage-5.10.beta2/local/lib/python2.7/site-packages/sage/combinat/partition.py", line 4043, in _element_constructor_
        raise ValueError('%s is not an element of %s'%(lst, self))
    ValueError: ['s', 'o', 'm', 'e', 't', 'h', 'i', 'n', 'g'] is not an element of Partitions
**********************************************************************

[tscrim]

comment:14

Attachment: trac_14145-fix_contains_tableau-ts.patch.gz

Fixed. Sorry about that Jeroen.

[jdemeyer]

Merged: sage-5.10.beta2