How do I overwrite comparison for modules?

[Simon Brandhorst]

Hi all,

I want to redefine comparison for modules by using python like comparisons
__eq__, __lt__ etc.

See #23978
https://trac.sagemath.org/ticket/23978

So I have deleted the __richcmp__ methods and added a method

    def __eq__(self, other):
        return type(self) == type(other)


I get the following error in sage:

See #23978
https://trac.sagemath.org/ticket/23978

sage: A = ZZ^2
sage: A == A
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-2-17b2cdf5c541> in <module>()
----> 1 A == A

/home/agag/sbrandhorst/sage/sage/src/sage/structure/richcmp.pyx in sage.structure.richcmp.slot_tp_richcompare (build/cythonized/sage/structure/richcmp.c:1414)()
    110     Function to put in the ``tp_richcompare`` slot.
    111     """
--> 112     return self.__richcmp__(other, op)
    113
    114

/home/agag/sbrandhorst/sage/sage/src/sage/structure/category_object.pyx in sage.structure.category_object.CategoryObject.__getattr__ (build/cythonized/sage/structure/category_object.c:7910)()
    846             AttributeError: 'PrimeNumbers_with_category' object has no attribute 'sadfasdf'
    847         """
--> 848         return self.getattr_from_category(name)
    849
    850     cdef getattr_from_category(self, name):

/home/agag/sbrandhorst/sage/sage/src/sage/structure/category_object.pyx in sage.structure.category_object.CategoryObject.getattr_from_category (build/cythonized/sage/structure/category_object.c:8073)()
    861                 cls = self._category.parent_class
    862
--> 863             attr = getattr_from_other_class(self, cls, name)
    864             self.__cached_methods[name] = attr
    865             return attr

/home/agag/sbrandhorst/sage/sage/src/sage/cpython/getattr.pyx in sage.cpython.getattr.getattr_from_other_class (build/cythonized/sage/cpython/getattr.c:1831)()
    247         dummy_error_message.cls = type(self)
    248         dummy_error_message.name = name
--> 249         raise dummy_attribute_error
    250     cdef PyObject* attr = _PyType_Lookup(<type>cls, name)
    251     if attr is NULL:

AttributeError: 'FreeModule_ambient_pid_with_category' object has no attribute '__richcmp__'


So somehow it insists on using __richcmp__. How can I teach it to use the python style methods?

[Jeroen Demeyer]

Remove the @richcmp_method decorator.

[Simon Brandhorst]

It works. Thank you :-).

On Monday, October 16, 2017 at 2:28:00 PM UTC+2, Jeroen Demeyer wrote:

Remove the @richcmp_method decorator.

[Travis Scrimshaw]

Now the question is why are you doing this? You have rich comparison behavior (i.e., a partial order) by allowing a return value of NotImplemented. So I do not understand why you would do this because it comes with a lot of boilerplate code and documentation unless you want to remove all use of inequality comparisons. Yet, I do not think that is what you are going to do.

Best,
Travis

[Simon Brandhorst]

Because the documentation did not tell me it was possible:

http://doc.sagemath.org/html/en/reference/structure/sage/structure/richcmp.html
Can you update it?

I have interpreted it as a way to go to python3 without changing the logic of the code.

[Daniel Krenn]

On 2017-10-17 12:13, Simon Brandhorst wrote:
> Because the documentation did not tell me it was possible:

Maybe sage.structure.richcmp.richcmp_by_eq_and_lt is something for you
(see also L1100 in sage/rings/asymptotic/growth_group.py).

Daniel

[Jeroen Demeyer]

On 2017-10-17 12:13, Simon Brandhorst wrote:

> Because the documentation did not tell me it was possible:

That *what* is possible? @richcmp_method simply allows to define just
one comparison method __richcmp__ instead of six __eq__, __lt__, ...

However, the functionality of what that comparison method can do remains
the same.

[Simon Brandhorst]

I got that now. But at the time I thought __richcmp__ was just a fancy new name for the old __cmp__ in python in order to make it python 3 compatible.

Anyways. It would be nice to say something like that in the documentation. (Maybe I have just missed it and it is there already.)

"
@richcmp_method simply allows to define just
one comparison method __richcmp__ instead of six __eq__, __lt__, ... "
"

and add a reference to some working example somewhere in the sage library.
As many people will come across that file who are not programmers and do not speak cython.
And in future they will also not know about the __cmp__ method of python 2.7

[Travis Scrimshaw]

On Tuesday, October 17, 2017 at 9:55:25 AM UTC-5, Simon Brandhorst wrote:

I got that now. But at the time I thought __richcmp__ was just a fancy new name for the old __cmp__ in python in order to make it python 3 compatible.

Anyways. It would be nice to say something like that in the documentation. (Maybe I have just missed it and it is there already.)
"
@richcmp_method simply allows to define just
one comparison method __richcmp__ instead of six __eq__, __lt__, ... "
"
and add a reference to some working example somewhere in the sage library.
As many people will come across that file who are not programmers and do not speak cython.
And in future they will also not know about the __cmp__ method of python 2.7

In Python, those are called rich comparisons and returning NotImplemented is a Python thing for rich comparisons, including in things like __eq__. So it doesn't have to do with speaking Cython. IMO, anytime you want to find an example in the Sage library, grep is your friend and is more robust than in some place in the documentation that could (quickly) go out of date. However, I agree with you that the documentation could be improved by stating about returning NotImplemented and having a more complete example.

Best,

Travis

 

[Simon Brandhorst]

Well richcmp allows you to define just one method. But it does not save you any work at all if the order is partial.
Because then you have to distinguish the cases op = op_LE , op_GE, op_LT ... etc. .... so all that you save is documentation and everything is more obscure.

I also tried the richcmp_by_eq_and_lt in principle it is a nice idea and exactly what I need.
In practice it does not work. Well I could get it to work and all doctests in the free_module.py and free_quadratic_module.py passed. But when testing the whole module folder everything exploded.
With commit b59062520b9b8e99e8ee126cbf209feb241a596a

    TypeError: _eq_() takes exactly 2 arguments (1 given)
    TypeError: unbound method _eq_() must be called with FreeModule_submodule_pid_with_category instance as first argument (got FreeModule_submodule_with_basis_pid_with_category instance instead)
    TypeError: unbound method _eq_() must be called with FreeModule_submodule_field_with_category instance as first argument (got FreeModule_submodule_with_basis_field_with_category instance instead)
etc. etc.

So my solution was to remove the @richcmp decorator and the richcmp_by_eq method. Then implement __neq__, __geq__ __leq__ by hand.
You can see it in #23978.

It needs review :).

So I expect that the richcmp_by_eq_and_lt does not work properly?
Also lt is harder to implement they le. That would be nice to have too?

[Simon Brandhorst]

/home/simon/sage/src/sage/structure/richcmp.pyx in sage.structure.richcmp.richcmp_by_eq_and_lt.richcmp (build/cythonized/sage/structure/richcmp.c:2172)()
    317             if equal_types:
    318                 other_eq = getattr(other, eq_attr)
--> 319             if other_eq(self):
    320                 return rich_to_bool(op, 0)
    321             if op == Py_EQ:

TypeError: unbound method _eq_() must be called with FreeModule_ambient_field_with_category instance as first argument (got FreeModule_submodule_field_with_category instance instead)

probably there should be
319        if other.other_eq(self)
or maybe
319        other.__eq__(self)

Also when having many different subclasses which have to compare this lt_by_eq_and_lt does not work. As it gives the comparison to the other element all the time. But I think that is really something that the user should decide in his code if he wants it. This does too much.
For me this resulted in circular references etc... so it was a pain to get it work.

        if not equal_types:
            try:
                other_eq = getattr(other, eq_attr)
                other_lt = getattr(other, lt_attr)

[Jeroen Demeyer]

If you say that something does not work, it would be good to show a
simple example of what you did. It is impossible for me to understand
why your code doesn't work without seeing your code.

[Simon Brandhorst]

from sage.structure.richcmp import richcmp_by_eq_and_lt,richcmp_method


@richcmp_method
class A(object):
    def __init__(self,x):
        self._x = x
    __richcmp__ = richcmp_by_eq_and_lt("_eq","_lt")
    def _eq(self,other):
        if type(other)!=type(self):
            #other knows how to do the comparison
            return other.__eq__(self)
        return self._x == other._x
    def _lt(self,other):
        return self._x<other._x
    def __repr__(self):
        return "A %r" %self._x
@richcmp_method

class B(A):
    #I can do comparison myself
    def __init__(self,x,extra):
        A.__init__(self,x)
   
    __richcmp__ = richcmp_by_eq_and_lt("_eq","_lt")
    def _eq(self,other):
        print("hello")
        return self._x == other._x
    def _lt(self,other):
        return self._x<other._x

    def __repr__(self):
        return "B %r" %self._x

There you go:

sage: A(1) == B(1,"a")
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
<ipython-input-4-f5a54e86b883> in <module>()
----> 1 A(Integer(1)) == B(Integer(1),"a")

/home/simon/sage/src/sage/structure/richcmp.pyx in sage.structure.richcmp.slot_tp_richcompare (build/cythonized/sage/structure/richcmp.c:1459)()

    110     Function to put in the ``tp_richcompare`` slot.
    111    

"""

--> 112     return self.__richcmp__(other, op)
    113
    114

/home/simon/sage/src/sage/structure/richcmp.pyx in sage.structure.richcmp.richcmp_by_eq_and_lt.richcmp (build/cythonized/sage/structure/richcmp.c:2178)()

    317             if equal_types:
    318                 other_eq = getattr(other, eq_attr)
--> 319             if other_eq(self):
    320                 return rich_to_bool(op, 0)
    321             if op == Py_EQ:

/home/simon/sage/src/sage/modules/bug.py in _eq(self, other)
     10         if type(other)!=type(self):
     11             #other knows how to do the comparison
---> 12             return other.__eq__(self)
     13         return self._x == other._x
     14     def _lt(self,other):

... last 3 frames repeated, from the frame below ...

/home/simon/sage/src/sage/structure/richcmp.pyx in sage.structure.richcmp.slot_tp_richcompare (build/cythonized/sage/structure/richcmp.c:1459)()

    110     Function to put in the ``tp_richcompare`` slot.
    111     """

--> 112     return self.__richcmp__(other, op)
    113
    114

RuntimeError: maximum recursion depth exceeded while calling a Python object
Enter code here...

This is unexpected but we know what to do

from sage.structure.richcmp import richcmp_by_eq_and_lt,richcmp_method


@richcmp_method
class A(object):
    def __init__(self,x):
        self._x = x
    __richcmp__ = richcmp_by_eq_and_lt("_eq","_lt")
    def _eq(self,other):
        if type(other)!=type(self):
            #other knows how to do the comparison
            return self.__eq__(other)
        return self._x == other._x
    def _lt(self,other):
        return self._x<other._x
    def __repr__(self):
        return "A %r" %self._x
@richcmp_method

class B(A):
    #I can do comparison myself
    def __init__(self,x,extra):
        A.__init__(self,x)
   
    __richcmp__ = richcmp_by_eq_and_lt("_eq","_lt")
    def _eq(self,other):
        print("hello")
        return self._x == other._x
    def _lt(self,other):
        return self._x<other._x

    def __repr__(self):
        return "B %r" %self._x

### reloading attached file bug.py modified at 12:00:42 ###
sage: A(1) == B(1,"a")
hello
True

[Simon Brandhorst]

For the second issue unfortunately I cannot find a simple example.
But I can reproduce it.
replace your src/sage/modules/free_module.py

by the file attached and recompile.
then go to sage and do:

sage: V=GF(7)^1
sage: phi=V.hom([V([1])])
sage: phi(V)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-15-3324c9a5d342> in <module>()
----> 1 phi(V)

/home/simon/sage/src/sage/categories/map.pyx in sage.categories.map.Map.__call__ (build/cythonized/sage/categories/map.c:6738)()
    785             return self._call_with_args(x, args, kwds)
    786         # Is there coercion?
--> 787         converter = D._internal_coerce_map_from(P)
    788         if converter is None:
    789             try:

/home/simon/sage/src/sage/structure/parent.pyx in sage.structure.parent.Parent._internal_coerce_map_from (build/cythonized/sage/structure/parent.c:18250)()
   2099             return mor
   2100
-> 2101         if S == self:
   2102             # non-unique parents
   2103             if debug.unique_parent_warnings:

/home/simon/sage/src/sage/structure/richcmp.pyx in sage.structure.richcmp.slot_tp_richcompare (build/cythonized/sage/structure/richcmp.c:1459)()

    110     Function to put in the ``tp_richcompare`` slot.
    111    

"""

--> 112     return self.__richcmp__(other, op)
    113
    114

/home/simon/sage/src/sage/structure/richcmp.pyx in sage.structure.richcmp.richcmp_by_eq_and_lt.richcmp (build/cythonized/sage/structure/richcmp.c:2172)()
    317             if equal_types:
    318                 other_eq = getattr(other, eq_attr)
--> 319             if other_eq(self):
    320                 return rich_to_bool(op, 0)
    321             if op == Py_EQ:

TypeError: _eq_() takes exactly 2 arguments (1 given)

By the way the tests in free_module.py pass. So comparison seems to be working.

[Travis Scrimshaw]

On Thursday, October 19, 2017 at 8:59:17 PM UTC-5, Simon Brandhorst wrote:

Well richcmp allows you to define just one method. But it does not save you any work at all if the order is partial.
Because then you have to distinguish the cases op = op_LE , op_GE, op_LT ... etc. .... so all that you save is documentation and everything is more obscure.

It depends. In fact, it can make the code easier to follow and maintain because you can do common cases for NE or for LT and LE. In fact, I might be so bold as to say that is what happens generally (how often is the first thing you do is verify the object has a compatible type?).

Best,

Travis

[Simon Brandhorst]

Mhh okay, I can see that point.

[Simon King]

Hi Travis,

On 2017-10-23, Travis Scrimshaw <tsc...@ucdavis.edu> wrote:
> (how often is the first
> thing you do is verify the object has a compatible type?).

Almost never, at least for elements!

For elements (not for parents, though), one is supposed to
implement single underscore arithmetic *and* comparison methods,
which will only be called after coercion took place and so the
two arguments of _richcmp_ are guaranteed to belong to the
same parent.

So, verifying that the types are compatible only applies to
the rare case that elements of a single parent are implemented
by different types, simultaneously. Just never ever override
double underscore __richtcmp__ for elements.

Cheers,
Simon

[Jeroen Demeyer]

This is an infinite loop:

....: if type(other)!=type(self):
....: #other knows how to do the comparison
....: return other.__eq__(self)

The correct Python thing to do is to return NotImplemented in this case.

[Simon Brandhorst]

I am aware of this. It is just unexpected that other.__eq__(self) calls
self._eq(other)

As a user of that rich cmp method I would have expected it to call
other._eq(self).

Raising a not implemented error seems nice. :)
Anyways I believe it should be better documented if you want people to use these functions.

[David Roe]

On Mon, Oct 23, 2017 at 8:24 AM, Simon Brandhorst <sbran...@web.de> wrote:

I am aware of this. It is just unexpected that other.__eq__(self) calls
self._eq(other)

As a user of that rich cmp method I would have expected it to call
other._eq(self).

Raising a not implemented error seems nice. :)

Note that it should return NotImplemented (a built in constant to be returned by rich comparisons), not raise a NotImplementedError.

David

[Travis Scrimshaw]

I am aware of this. It is just unexpected that other.__eq__(self) calls
self._eq(other)

As a user of that rich cmp method I would have expected it to call
other._eq(self).

Raising a not implemented error seems nice. :)

Note that it should return NotImplemented (a built in constant to be returned by rich comparisons), not raise a NotImplementedError.

There is also precedent in Python for raising a TypeError with < and > too. However, I think that causes more headaches than it solves.

Best,

Travis

[Travis Scrimshaw]

> (how often is the first
> thing you do is verify the object has a compatible type?).

Almost never, at least for elements!

For elements (not for parents, though), one is supposed to
implement single underscore arithmetic *and* comparison methods,
which will only be called after coercion took place and so the
two arguments of _richcmp_ are guaranteed to belong to the
same parent.

So, verifying that the types are compatible only applies to
the rare case that elements of a single parent are implemented
by different types, simultaneously. Just never ever override
double underscore __richtcmp__ for elements.

Yes, that is a very good point to make and reinforce.

Best,

Travis

 

[Simon Brandhorst]

Ahhh

[Simon Brandhorst]

Okay I made it work with __richcmp__ as suggested.

Please review :-) .

[Jeroen Demeyer]

> Just never ever override
> double underscore __richtcmp__ for elements.
>
> Yes, that is a very good point to make and reinforce.

Same for __add__, __mul__ and so on... except if you explicitly want to
bypass the coercion model (which is not fully supported anyway, see
#24066)