Opened 9 years ago

Closed 9 years ago

#14145 closed defect (fixed)

Problems with contains for Tableau, TableauTuples and PartitionTuples

Reported by: andrew.mathas Owned by: sage-combinat
Priority: major Milestone: sage-5.10
Component: combinatorics Keywords: days45
Cc: andrew.mathas Merged in: sage-5.10.beta2
Authors: Travis Scrimshaw, Andrew Mathas Reviewers: Andrew Mathas, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #13605 Stopgaps:

Status badges

Description

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.

Attachments (1)

trac_14145-fix_contains_tableau-ts.patch (85.8 KB) - added by tscrim 9 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 9 years ago by andrew.mathas

  • Keywords days45 added; sage45 removed

comment:2 Changed 9 years ago by tscrim

  • Authors set to Travis Scrimshaw
  • Dependencies set to #13605
  • Status changed from new to needs_review

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

comment:3 Changed 9 years ago by andrew.mathas

  • Reviewers set to Andrew Mathas

comment:4 Changed 9 years ago by andrew.mathas

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

comment:5 Changed 9 years ago by tscrim

Hey Andrew,

Just wondering what the status of the review patch is.

Thanks,
Travis

comment:6 Changed 9 years ago by andrew.mathas

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

comment:7 Changed 9 years ago by tscrim

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

comment:8 Changed 9 years ago by andrew.mathas

  • Authors changed from Travis Scrimshaw to Travis Scrimshaw, Andrew Mathas
  • Reviewers changed from Andrew Mathas to Andrew Mathas, Travis Scrimshaw

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

comment:9 Changed 9 years ago by tscrim

  • Status changed from needs_review to positive_review

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

comment:10 Changed 9 years ago by jdemeyer

  • Status changed from positive_review to needs_info

Why the change from

except StandardError:

to

except:

comment:11 Changed 9 years ago by tscrim

  • Status changed from needs_info to needs_review

Sorry missed that one.

comment:12 Changed 9 years ago by tscrim

  • Status changed from needs_review to positive_review

comment:13 Changed 9 years ago by jdemeyer

  • Status changed from positive_review to needs_work
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
**********************************************************************

Changed 9 years ago by tscrim

comment:14 Changed 9 years ago by tscrim

  • Status changed from needs_work to positive_review

Fixed. Sorry about that Jeroen.

comment:15 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.10.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.