Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Element overrides python behavior of cmp #14065

Closed
tscrim opened this issue Feb 6, 2013 · 12 comments
Closed

Element overrides python behavior of cmp #14065

tscrim opened this issue Feb 6, 2013 · 12 comments

Comments

@tscrim
Copy link
Collaborator

tscrim commented Feb 6, 2013

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.

Depends on #14052

CC: @nthiery @sagetrac-sage-combinat

Component: combinatorics

Keywords: Element, cmp, days45

Author: Travis Scrimshaw

Reviewer: Mike Hansen

Merged: sage-5.8.beta0

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

@tscrim
Copy link
Collaborator Author

tscrim commented Feb 6, 2013

Dependencies: #14052

@tscrim
Copy link
Collaborator Author

tscrim commented Feb 6, 2013

comment:2

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

@tscrim

This comment has been minimized.

@tscrim
Copy link
Collaborator Author

tscrim commented Feb 11, 2013

Attachment: trac_14065-combinatorial_object_cmp-ts.patch.gz

Expanded doctests

@tscrim

This comment has been minimized.

@tscrim
Copy link
Collaborator Author

tscrim commented Feb 11, 2013

comment:4

Added additional doctests about the MRO.

@mwhansen
Copy link
Contributor

@mwhansen
Copy link
Contributor

comment:5

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.

@tscrim
Copy link
Collaborator Author

tscrim commented Feb 13, 2013

Reviewer: Mike Hansen

@tscrim
Copy link
Collaborator Author

tscrim commented Feb 13, 2013

comment:6

Looks good to me. Thank you Mike.

@tscrim
Copy link
Collaborator Author

tscrim commented Feb 14, 2013

Changed keywords from Element, cmp to Element, cmp, days45

@jdemeyer jdemeyer modified the milestones: sage-5.7, sage-5.8 Feb 14, 2013
@jdemeyer
Copy link
Contributor

Merged: sage-5.8.beta0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants