Opened 10 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: |
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)
Change History (16)
comment:1 Changed 10 years ago by
- Keywords days45 added; sage45 removed
comment:2 Changed 9 years ago by
- Dependencies set to #13605
- Status changed from new to needs_review
comment:3 Changed 9 years ago by
- Reviewers set to Andrew Mathas
comment:4 Changed 9 years ago by
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
Hey Andrew,
Just wondering what the status of the review patch is.
Thanks,
Travis
comment:6 Changed 9 years ago by
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
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
- 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
- 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
- Status changed from positive_review to needs_info
Why the change from
except StandardError:
to
except:
comment:11 Changed 9 years ago by
- Status changed from needs_info to needs_review
Sorry missed that one.
comment:12 Changed 9 years ago by
- Status changed from needs_review to positive_review
comment:13 Changed 9 years ago by
- 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
comment:14 Changed 9 years ago by
- Status changed from needs_work to positive_review
Fixed. Sorry about that Jeroen.
comment:15 Changed 9 years ago by
- Merged in set to sage-5.10.beta2
- Resolution set to fixed
- Status changed from positive_review to closed
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 toin
throws an error, it returnsFalse
...I think I've probably said more than enough. Anyways, this patch fixes the problem at hand and is ready for review.Best,
Travis