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 tscrim)

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)

trac_14065-combinatorial_object_cmp-ts.patch (5.9 KB) - added by tscrim 6 years ago.
Expanded doctests
trac_14065-combinatorial_object_cmp-review-mh.patch (1.2 KB) - added by mhansen 6 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 6 years ago by tscrim

  • Status changed from new to needs_review

comment:2 Changed 6 years ago by tscrim

  • Dependencies set to #14052

Dependency on #14052 is due to an added doctest there that changes.

comment:3 Changed 6 years ago by tscrim

  • Description modified (diff)

Changed 6 years ago by tscrim

Expanded doctests

comment:4 Changed 6 years ago by tscrim

  • Description modified (diff)

Added additional doctests about the MRO.

comment:5 Changed 6 years ago by mhansen

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 tscrim

  • 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 tscrim

  • Keywords days45 added

comment:8 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.7 to sage-5.8

comment:9 Changed 6 years ago by jdemeyer

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