Opened 6 years ago
Closed 6 years ago
#14065 closed defect (fixed)
Element overrides python behavior of cmp
Reported by: | tscrim | Owned by: | sage-combinat |
---|---|---|---|
Priority: | major | Milestone: | sage-5.8 |
Component: | combinatorics | Keywords: | Element, cmp, days45 |
Cc: | nthiery, sage-combinat | Merged in: | sage-5.8.beta0 |
Authors: | Travis Scrimshaw | Reviewers: | Mike Hansen |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #14052 | Stopgaps: |
Description (last modified by )
As part of #12913, we are adding inheritance from Element
to many classes that inherit from CombinatorialObject
. However, there are many functions are relying on a valid call to cmp
, but Element
overrides this. This patch is a fix which implements a basic __cmp__
for CombinatorialObject
for the switch.
Another issue this ticket does is add a __nonzero__()
to CombinatorialObject
so things like if p:
and not p
will work when also inheriting from Element
(which checks against the Parent().zero_element()
and is not implemented for all parents).
Here's an example of how this breaks:
sage: from sage.structure.element import Element sage: class Foo(CombinatorialObject, Element): ... def __init__(self, l): ... CombinatorialObject.__init__(self, l) ... sage: L = [Foo([4-i]) for i in range(4)] sage: sorted(L, cmp) Traceback (most recent call last): ... NotImplementedError: BUG: sort algorithm for elements of 'None' not implemented sage: f = Foo([4]) sage: not f Traceback (most recent call last): ... AttributeError: 'NoneType' object has no attribute 'zero_element'
NOTE - After the patch, the CombinatorialObject
needs to come before Element
because of the MRO.
Attachments (2)
Change History (11)
comment:1 Changed 6 years ago by
- Status changed from new to needs_review
comment:2 Changed 6 years ago by
- Dependencies set to #14052
comment:3 Changed 6 years ago by
- Description modified (diff)
comment:4 Changed 6 years ago by
- Description modified (diff)
Added additional doctests about the MRO.
Changed 6 years ago by
comment:5 Changed 6 years ago by
Looks good to me.
I've added a review patch which simplifies the implementation a bit. If you're fine with it, you can set this to positive review.
comment:6 Changed 6 years ago by
- Reviewers set to Mike Hansen
- Status changed from needs_review to positive_review
Looks good to me. Thank you Mike.
comment:7 Changed 6 years ago by
- Keywords days45 added
comment:8 Changed 6 years ago by
- Milestone changed from sage-5.7 to sage-5.8
comment:9 Changed 6 years ago by
- Merged in set to sage-5.8.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
Dependency on #14052 is due to an added doctest there that changes.