Opened 7 years ago

Closed 7 years ago

#17890 closed enhancement (fixed)

Remove _(rich)cmp_c_impl

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.7
Component: cython Keywords:
Cc: dkrenn Merged in:
Authors: Jeroen Demeyer Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 3976f2c (Commits, GitHub, GitLab) Commit: 3976f2cc4ec59c9f978d9d02c7f697c81025c4ef
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

Instead of

    def _cmp_(left, right)
    cdef int _cmp_c_impl(left, right) except -2

we should have

    cpdef int _cmp_(left, right) except -2

Analogously for _richcmp_.

There is one important functional change: If a Python class defines both __cmp__ and _cmp_, then formerly the coercion framework would use __cmp__ by default to implement relational operators like >=. After this ticket, _cmp_ will be tried first, which defaults to __cmp__.

Change History (70)

comment:1 Changed 7 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Remove _cmp_c_impl to Remove _(rich)cmp_c_impl

comment:2 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:4 Changed 7 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/17890

comment:5 Changed 7 years ago by jdemeyer

  • Commit set to a830cc93c6bb07565db709bfe03f8acc62034dbb
  • Description modified (diff)

New commits:

7083f19Remove use of PY_IS_NUMERIC
7eb0e93Move py_scalar_to_element to coerce.pyx
8300f59trac #17862 (review): doc typo
a3ea57fMerge tag '6.6.beta2' into HEAD
a830cc9Remove _cmp_c_impl and _richcmp_c_impl

comment:6 follow-up: Changed 7 years ago by vdelecroix

If it is slower this is very bad. Do you know why? Isn't a cdef function call equivalent to cpdef function call?

Vincent

comment:7 in reply to: ↑ 6 Changed 7 years ago by jdemeyer

Replying to vdelecroix:

If it is slower this is very bad. Do you know why?

Yes, see https://groups.google.com/forum/#!topic/sage-devel/AXYk1uEfuPE

comment:8 Changed 7 years ago by mmezzarobba

Related: #10130.

comment:9 Changed 7 years ago by git

  • Commit changed from a830cc93c6bb07565db709bfe03f8acc62034dbb to 25e517fc735d15ba07c919f7a65103cf04acd2cf

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

25e517fRemove _cmp_c_impl and _richcmp_c_impl

comment:10 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:11 Changed 7 years ago by jdemeyer

  • Dependencies set to #17862

comment:12 Changed 7 years ago by git

  • Commit changed from 25e517fc735d15ba07c919f7a65103cf04acd2cf to 41f7cd14911ed5fde23731c2c78f235e481258e6

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

41f7cd1Remove _cmp_c_impl and _richcmp_c_impl

comment:13 Changed 7 years ago by jdemeyer

  • Status changed from new to needs_review

comment:14 Changed 7 years ago by vdelecroix

See also #16537

comment:15 Changed 7 years ago by git

  • Commit changed from 41f7cd14911ed5fde23731c2c78f235e481258e6 to d26954c093a671139347c65ee7772b79e9a02783

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

d26954cRemove _cmp_c_impl and _richcmp_c_impl

comment:16 Changed 7 years ago by jdemeyer

  • Dependencies #17862 deleted

comment:17 Changed 7 years ago by git

  • Commit changed from d26954c093a671139347c65ee7772b79e9a02783 to ea13fd01a8a37ce5d6d42e58d6c5f047250d3f38

Branch pushed to git repo; I updated commit sha1. New commits:

ea13fd0Merge tag '6.6.beta5' into t/17890/ticket/17890

comment:18 Changed 7 years ago by jdemeyer

  • Cc dkrenn added

comment:19 Changed 7 years ago by git

  • Commit changed from ea13fd01a8a37ce5d6d42e58d6c5f047250d3f38 to a7cbb88852ceefff65e421e75c0479bcd554ce26

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a7cbb88Remove _cmp_c_impl and _richcmp_c_impl

comment:20 follow-ups: Changed 7 years ago by vdelecroix

  • Status changed from needs_review to needs_info

sage.structure.element

  1. In the documentation of sage.structure.element emphasize what should be done for Python classes as well (and the potential difference between Python and Cython). It is not very clear to me! What I understood is that we should:
    • implement __cmp__(left,right) or/and _richcmp_(left,right,op)
    • avoid __eq__, __ne__, etc
  1. What about putting
    # Obvious case
    if left is right:
        return _rich_to_bool(op, 0)
    
    directly in __richcmp__?

sage.structure.parent

  1. The default comparison for parents is by id which is natural (and I guess what Python does). But the default hash is not based on id and is the stupid one inherited from SageObject. Is there a reason?
  1. Why not using _rich_to_bool in parent.pyx instead of
    if op == 0:  #< 
        return r  < 0
    elif op == 2: #==
        return r == 0 
    elif op == 4: #>
        return r  > 0
    elif op == 1: #<=
        return r <= 0
    elif op == 3: #!=
        return r != 0
    elif op == 5: #>=
        return r >= 0 
    
    And by the way, why is there a method cdef _rich_to_bool on element? it would be useful as a standalone function in other places.

comment:21 in reply to: ↑ 20 Changed 7 years ago by jdemeyer

Replying to vdelecroix:

sage.structure.element

  1. In the documentation of sage.structure.element emphasize what should be done for Python classes as well (and the potential difference between Python and Cython). It is not very clear to me!

Unfortunately, it is not very clear to me either. In particular, I don't find good Python documentation about the interaction between __cmp__, rich comparisons like __eq__ and __hash__ and what Python does when you call cmp(x,y). It seems that cmp(x,y) only calls x.__cmp__(y) when x and y have the same type, but I don't find this anywhere documented. And I know that inheritance works in a strange way for comparisons, but I also don't quite know how.

Last edited 7 years ago by jdemeyer (previous) (diff)

comment:22 in reply to: ↑ 20 Changed 7 years ago by jdemeyer

Replying to vdelecroix:

  1. What about putting
    # Obvious case
    if left is right:
        return _rich_to_bool(op, 0)
    
    directly in __richcmp__?

Because __richcmp__ should absolutely be as simple as possible. Don't forget that every subclass needs a __richcmp__ method (because of Python's strange inheritance rules regarding comparison).

comment:23 in reply to: ↑ 20 Changed 7 years ago by jdemeyer

Replying to vdelecroix:

  1. The default comparison for parents is by id which is natural (and I guess what Python does). But the default hash is not based on id and is the stupid one inherited from SageObject. Is there a reason?

I don't know. This ticket doesn't change anything to hashing, so I consider this outside the scope of this ticket (but you might have a point though).

comment:24 Changed 7 years ago by git

  • Commit changed from a7cbb88852ceefff65e421e75c0479bcd554ce26 to 1ad339bf0c8ffe75d23ddaa6d896121b8c96636d

Branch pushed to git repo; I updated commit sha1. New commits:

9b48fecDoctest .pxd files also
1ad339bImplement _rich_to_bool as inline function instead of member function

comment:25 Changed 7 years ago by git

  • Commit changed from 1ad339bf0c8ffe75d23ddaa6d896121b8c96636d to 17bd067153ad40ced061147390687f514a7f7cba

Branch pushed to git repo; I updated commit sha1. New commits:

17bd067Merge tag '6.7.beta2' into t/17890/ticket/17890

comment:26 Changed 7 years ago by jdemeyer

  • Status changed from needs_info to needs_review

OK, I improved rich_to_bool a lot.

comment:27 follow-up: Changed 7 years ago by vdelecroix

I did some experimentations with Python classes. The conclusion is:

just define _cmp_ and _richcmp_ (even though you redefine __hash__)

Then:

  • rich comparisons and comparisons work when it involves elements with the same parent (i.e. the newly defined _cmp_ and _richcmp_ gets called)
  • rich comparisons involving coercion is fine
  • comparison involving coercion is wrong: if you call cmp(a,b) with a and b between two parents A and B and a coercion B -> A then Element.__richcmp__ is called instead of Element.__cmp__.

Actually, this is not only a bug in Python but also in Cython! I added print statement in each comparisons function in Element and see

sage: cmp(1,1/3)
Element._richcmp(1,1/3,2)
Element._richcmp(1,1/3,2)
Element._richcmp_(1,1/3,2)
Element._richcmp(1,1/3,0)
Element._richcmp(1,1/3,0)
Element._richcmp_(1,1/3,0)
Element._richcmp(1,1/3,4)
Element._richcmp(1,1/3,4)
Element._richcmp_(1,1/3,4)
1

Vincent

comment:28 follow-ups: Changed 7 years ago by vdelecroix

Moreover if you redefine __cmp__ in your Element class then Element.__cmp__ is not called! So the test in Element._cmp seems useless.

comment:29 in reply to: ↑ 28 Changed 7 years ago by vdelecroix

Replying to vdelecroix:

Moreover if you redefine __cmp__ in your Element class then Element.__cmp__ is not called! So the test in Element._cmp seems useless.

Except if you do not define _richcmp_ and do some rich comparisons (this is the case in sage.rings.qqbar.AlgebraicReal for example).

comment:30 in reply to: ↑ 27 ; follow-up: Changed 7 years ago by jdemeyer

Replying to vdelecroix:

  • comparison involving coercion is wrong: if you call cmp(a,b) with a and b between two parents A and B and a coercion B -> A then Element.__richcmp__ is called instead of Element.__cmp__.

Actually, this is not only a bug in Python

Bug or feature?

comment:31 Changed 7 years ago by vdelecroix

And implementing _richcmp_ instead of __cmp__ in AlgebraicReal I got an incredible speedup (see #18303).

comment:32 in reply to: ↑ 28 ; follow-up: Changed 7 years ago by jdemeyer

Replying to vdelecroix:

Moreover if you redefine __cmp__ in your Element class then Element.__cmp__ is not called!

Of course not, that's a general rule for inheritance.

So the test in Element._cmp seems useless.

What do you mean?

comment:33 in reply to: ↑ 32 ; follow-up: Changed 7 years ago by vdelecroix

Replying to jdemeyer:

Replying to vdelecroix:

Moreover if you redefine __cmp__ in your Element class then Element.__cmp__ is not called!

Of course not, that's a general rule for inheritance.

I thought this was not the case for cython classes which redefine __hash__! Is it only true for __richcmp__?

So the test in Element._cmp seems useless.

What do you mean?

Sorry, I forgot a _. I wanted to say Element._cmp_. The default implementation does check whether __cmp__ is redefined in element classes. This code is called only if __cmp__ is defined but not _richcmp_. And it seems much better to define _richcmp_ in children classes.

Vincent

comment:34 in reply to: ↑ 30 ; follow-up: Changed 7 years ago by vdelecroix

Replying to jdemeyer:

Replying to vdelecroix:

  • comparison involving coercion is wrong: if you call cmp(a,b) with a and b between two parents A and B and a coercion B -> A then Element.__richcmp__ is called instead of Element.__cmp__.

Actually, this is not only a bug in Python

Bug or feature?

Call it whatever you want: cmp(a1,a2) (same type) calls __cmp__ but cmp(a,b) (different types) does not and use __richcmp__ instead. The consequence is that you might perform three times the coercion while calling cmp with elements with different parents!

comment:35 in reply to: ↑ 34 ; follow-up: Changed 7 years ago by jdemeyer

Replying to vdelecroix:

Call it whatever you want: cmp(a1,a2) (same type) calls __cmp__ but cmp(a,b) (different types) does not and use __richcmp__ instead. The consequence is that you might perform three times the coercion while calling cmp with elements with different parents!

I agree. Unfortunately this is general Python behaviour and not something this ticket can fix.

comment:36 in reply to: ↑ 33 ; follow-up: Changed 7 years ago by jdemeyer

Replying to vdelecroix:

Replying to jdemeyer:

Replying to vdelecroix:

Moreover if you redefine __cmp__ in your Element class then Element.__cmp__ is not called!

Of course not, that's a general rule for inheritance.

I thought this was not the case for cython classes which redefine __hash__! Is it only true for __richcmp__?

I don't understand what you are saying. For any method foo, the statement "if you redefine foo in your Element class then Element.foo is not called" is true.

comment:37 in reply to: ↑ 36 ; follow-up: Changed 7 years ago by vdelecroix

Replying to jdemeyer:

Replying to vdelecroix:

Replying to jdemeyer:

Replying to vdelecroix:

Moreover if you redefine __cmp__ in your Element class then Element.__cmp__ is not called!

Of course not, that's a general rule for inheritance.

I thought this was not the case for cython classes which redefine __hash__! Is it only true for __richcmp__?

I don't understand what you are saying. For any method foo, the statement "if you redefine foo in your Element class then Element.foo is not called" is true.

Why in extension class one has to copy/paste the definition of __richcmp__?

comment:38 in reply to: ↑ 35 Changed 7 years ago by vdelecroix

Replying to jdemeyer:

Replying to vdelecroix:

Call it whatever you want: cmp(a1,a2) (same type) calls __cmp__ but cmp(a,b) (different types) does not and use __richcmp__ instead. The consequence is that you might perform three times the coercion while calling cmp with elements with different parents!

I agree. Unfortunately this is general Python behaviour and not something this ticket can fix.

It needs to be emphasized in the doc then. We should try hard to avoid cmp(x,y)!

comment:39 follow-up: Changed 7 years ago by vdelecroix

  • Status changed from needs_review to needs_work
  1. For Python classes, I guess that one needs to implement _cmp_ and/or _richcmp_. This must be precised in the doc of Element/Parent. For this reason, we need a py_rich_to_bool available at the level of Python (as well as the operators Py_EQ, Py_NE, etc).
  1. I think we should avoid implenting __eq__, __ne__ because one can do with _richcmp_ that would take care of coercions. What do you think?
  1. There should be a mention of the Python issue that cmp(a,b) with a and b of different types does not call _cmp_ (or __cmp__)

comment:40 follow-up: Changed 7 years ago by vdelecroix

  1. For right_to_bool_sgn:
    • I found the following more readable
      cdef inline bint rich_to_bool_sgn(int op, long c):
          return rich_to_bool(op, (c > 0) - (c < 0))
      
      and in Integer._cmp_, you could optimize the sign check as well (either with your version or the one above)
    • why the input is long c and not int c? the output of mpz_cmp is an int for example.

comment:41 in reply to: ↑ 37 Changed 7 years ago by jdemeyer

Replying to vdelecroix:

Why in extension class one has to copy/paste the definition of __richcmp__?

This is vaguely explained in https://docs.python.org/2/reference/datamodel.html#object.__hash__ (when reading this, mentally replace __eq__ by __richcmp__)

comment:42 Changed 7 years ago by jdemeyer

This is how Python implements cmp:

static int
do_cmp(PyObject *v, PyObject *w)
{
    int c;
    cmpfunc f;

    if (v->ob_type == w->ob_type
        && (f = v->ob_type->tp_compare) != NULL) {
        c = (*f)(v, w);
        if (PyInstance_Check(v)) {
            /* Instance tp_compare has a different signature.
               But if it returns undefined we fall through. */
            if (c != 2)
                return c;
            /* Else fall through to try_rich_to_3way_compare() */
        }
        else
            return adjust_tp_compare(c);
    }
    /* We only get here if one of the following is true:
       a) v and w have different types
       b) v and w have the same type, which doesn't have tp_compare
       c) v and w are instances, and either __cmp__ is not defined or
          __cmp__ returns NotImplemented
    */
    c = try_rich_to_3way_compare(v, w);
    if (c < 2)
        return c;
    c = try_3way_compare(v, w);
    if (c < 2)
        return c;
    return default_3way_compare(v, w);
}

Note how tp_compare is only called if the types of v and w are equal. If not, __richcmp__ is used. Only if __richcmp__ is not implemented, then __cmp__ is tried after all.

comment:43 in reply to: ↑ 40 ; follow-up: Changed 7 years ago by jdemeyer

Replying to vdelecroix:

  1. For right_to_bool_sgn:
    • I found the following more readable
      cdef inline bint rich_to_bool_sgn(int op, long c):
          return rich_to_bool(op, (c > 0) - (c < 0))
      

Sure, I can change that.

  • why the input is long c and not int c? the output of mpz_cmp is an int for example.

It's an inline function, so it won't matter much for performance. long is more general than int, so if there is ever a use case for a long, it will be possible.

comment:44 in reply to: ↑ 39 ; follow-up: Changed 7 years ago by jdemeyer

Replying to vdelecroix:

  1. For Python classes, I guess that one needs to implement _cmp_ and/or _richcmp_. This must be precised in the doc of Element/Parent. For this reason, we need a py_rich_to_bool available at the level of Python (as well as the operators Py_EQ, Py_NE, etc).
  1. I think we should avoid implenting __eq__, __ne__ because one can do with _richcmp_ that would take care of coercions. What do you think?

Can we please postpone the pure Python case to a new ticket? I feel like this ticket is already getting big enough and I would prefer to focus on Cython here.

comment:45 in reply to: ↑ 44 Changed 7 years ago by vdelecroix

Replying to jdemeyer:

Replying to vdelecroix:

  1. For Python classes, I guess that one needs to implement _cmp_ and/or _richcmp_. This must be precised in the doc of Element/Parent. For this reason, we need a py_rich_to_bool available at the level of Python (as well as the operators Py_EQ, Py_NE, etc).
  1. I think we should avoid implenting __eq__, __ne__ because one can do with _richcmp_ that would take care of coercions. What do you think?

Can we please postpone the pure Python case to a new ticket? I feel like this ticket is already getting big enough and I would prefer to focus on Cython here.

And there are already two tickets depending on it ;-) I opened #18305

Vincent

Last edited 7 years ago by vdelecroix (previous) (diff)

comment:46 Changed 7 years ago by jdemeyer

For the documentation, I opened #18306.

comment:47 in reply to: ↑ 43 Changed 7 years ago by jdemeyer

Replying to jdemeyer:

It's an inline function, so it won't matter much for performance. long is more general than int, so if there is ever a use case for a long, it will be possible.

Hmm, just checked the assembly output of GCC 4.8.4 and it doesn't actually optimize this properly. So I'll stick to int indeed.

comment:48 Changed 7 years ago by git

  • Commit changed from 17bd067153ad40ced061147390687f514a7f7cba to 313a400146b0fb19d80c31117d44ce735041d4ec

Branch pushed to git repo; I updated commit sha1. New commits:

313a400Optimize rich_to_bool_sgn

comment:49 Changed 7 years ago by jdemeyer

For the assembly literate: with this new branch,

return rich_to_bool(op, (c > 0) - (c < 0))

becomes

    xorl    %edx, %edx
    testl   %eax, %eax        # test c
    setg    %dl               # c > 0
    shrl    $31, %eax         # c < 0 computed as ((unsigned int)c) >> 31
    subl    %eax, %edx        # shift = (c > 0) - (c < 0)
    movl    $184563750, %eax  # bits = 184563750
    sall    $3, %edx          # shift *= 8
    addl    %r12d, %edx       # shift += op
    btl %edx, %eax            # test bit "shift" of "bits"
    jnc ...

New commits:

313a400Optimize rich_to_bool_sgn

comment:50 Changed 7 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:51 follow-up: Changed 7 years ago by vdelecroix

  • Status changed from needs_review to needs_info
  1. implementation of _(rich)cmp:
    • Why in _(rich)cmp it starts with the case not have_same_parent_c(left,right) instead of have_same_parent_c(left,right). It would make it more readable the other way around.
      cdef _richcmp(left, right):
          if have_same_parent_c(left, right):
              return left._richcmp_(<Element>right, op)
      
          # different parents
          global coercion_model
          cdef int r
          ...
      
    • In return left._(rich)cmp_(<Element>right, op) is the <Element> really needed or usefull?
    • could _left, _right = coercion_model.canonical_coercion(left, right) be replaced with left, right = coercion_model.canonical_coercion(left, right). As far as I see, there is no need of these extra variables.
  1. Could you some minimal documentation to _(rich)cmp and _(rich)cmp_ like
    def _richcmp(left, right):
        r"""
        Rich comparisons of elements
    
        This should not be overriden! To implement comparisons override ``_richcmp_``
        or/and ``_cmp_``.
        """
    
    def _richcmp_(left, right):
        r"""
        Default implementation of rich comparisons for elements
    
        By default it tries to see if `_cmp_` is implemented otherwise does a
        comparison by id for ``==`` and ``!=``. Calling this default method
        with ``<``, ``<=``, ``>`` or ``>=`` will raise a ``NotImplementedError``.
    
        EXAMPLES::
    
            sage: from sage.structure.parent import Parent
            sage: from sage.structure.element import Element
            sage: P = Parent()
            sage: e1 = Element(P); e2 = Element(P)
            sage: e1 == e1    # indirect doctest
            True
            sage: e1 == e2    # indirect doctest
            False
            sage: e1 < e2     # indirect doctest
            Traceback (most recent call last):
            ...
            NotImplementedError: comparison not implemented for
            <type 'sage.structure.element.Element'>
        """
    
    and I think that _cmp_ and _richcmp_ deserves a sphinx directive to make them appear in the reference manual.
Version 0, edited 7 years ago by vdelecroix (next)

comment:52 in reply to: ↑ 51 Changed 7 years ago by jdemeyer

Replying to vdelecroix:

  1. implementation of _(rich)cmp:
    • Why in _(rich)cmp it starts with the case not have_same_parent_c(left,right) instead of have_same_parent_c(left,right). It would make it more readable the other way around.

I don't know why it would be more readable, but I can change it.

  • In return left._(rich)cmp_(<Element>right, op) is the <Element> really needed or usefull?

It's useful for efficiency. Note how _(rich)cmp_ is declared with Element right. By explicitly calling this with <Element>right, Cython does not need to check that right is indeed an Element.

  • could _left, _right = coercion_model.canonical_coercion(left, right) be replaced with left, right = coercion_model.canonical_coercion(left, right). As far as I see, there is no need of these extra variables.

Perhaps surprisingly, there is a need at least for an extra variable _left. The point is that left is a "self" argument which Cython will always assume and require that it is an instance of Element.

comment:53 Changed 7 years ago by git

  • Commit changed from 313a400146b0fb19d80c31117d44ce735041d4ec to 0d1e049f807a1c34b00cf0e58ee65cc7734d1ad1

Branch pushed to git repo; I updated commit sha1. New commits:

629f6a5Improve comparisons for permutation groups
0d1e049Improve _richcmp and documentation

comment:54 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-6.6 to sage-6.7
  • Status changed from needs_info to needs_review

OK, I made some further changes to _richcmp. There is one important functional change: when comparing two elements of the same type but different parents such that no coercion is possible (e.g. vectors over a different finite field), comparison is done by id instead of always returning True for __lt__:

before:

sage: v = vector(GF(5), [1])
sage: w = vector(GF(7), [1])
sage: v < w
True
sage: w < v
True

after, id(v) and id(w) are compared.

This caused some doctest failures, which I simply removed (there was not much point in those tests). I also discovered #18308 this way (where there should be a coercion, but there isn't).

comment:55 follow-up: Changed 7 years ago by vdelecroix

You wrote in the doc of _richcmp

        If a class wants to implement rich comparison without coercion,
        then ``_richcmp`` should be defined (as well as ``__richcmp__``
        as usual).

Why not define directly __richcmp__ in that case? This is exactly what is done in Integer (and also Rational in #18304). These could be cited as examples BTW.

comment:56 in reply to: ↑ 55 ; follow-up: Changed 7 years ago by jdemeyer

Replying to vdelecroix:

You wrote in the doc of _richcmp

        If a class wants to implement rich comparison without coercion,
        then ``_richcmp`` should be defined (as well as ``__richcmp__``
        as usual).

Why not define directly __richcmp__ in that case?

Because rich comparison after coercion calls _richcmp, not __richcmp__. The only reason this works for Integer is that ZZ has no sub-rings: it is never the common parent of two different parents.

comment:57 Changed 7 years ago by jdemeyer

For Rational in #18304, a similar argument holds: the only coercion to QQ from a Sage Parent is from ZZ. Since you're not using the coercion framework for this comparison between Integer and Rational, you're safe.

In other words: both work for very specific reasons, they are not examples to follow.

comment:58 Changed 7 years ago by git

  • Commit changed from 0d1e049f807a1c34b00cf0e58ee65cc7734d1ad1 to 39273f1aacfdd307d20b729ae732e349438a8422

Branch pushed to git repo; I updated commit sha1. New commits:

39273f1Fix doctest formatting

comment:59 Changed 7 years ago by git

  • Commit changed from 39273f1aacfdd307d20b729ae732e349438a8422 to 04570b3dfd194fea8c2437c8717c3dfb8704b4af

Branch pushed to git repo; I updated commit sha1. New commits:

04570b3Fix bad doctest in etaproducts

comment:60 in reply to: ↑ 56 ; follow-up: Changed 7 years ago by vdelecroix

Replying to jdemeyer:

Replying to vdelecroix:

You wrote in the doc of _richcmp

        If a class wants to implement rich comparison without coercion,
        then ``_richcmp`` should be defined (as well as ``__richcmp__``
        as usual).

Why not define directly __richcmp__ in that case?

Because rich comparison after coercion calls _richcmp, not __richcmp__. The only reason this works for Integer is that ZZ has no sub-rings: it is never the common parent of two different parents.

Right, but _richcmp will call _richcmp_/_cmp_. In Integers (and Rationals after #18304) there is also this _cmp_ implemented and if everything else failed in __richcmp__ there is precisely Element._richcmp(self, other, op).

So even if you implement a subset of ZZ/QQ as a new parent (let say the additive group 3 ZZ or the ring ZZ[1/p]) then it would be fine with comparisons with integers/rationals as soon as you define a coercion 3 ZZ -> ZZ/ZZ[1/p] -> QQ. So my mistake, the situation for ZZ and QQ is a bit different: Integer/Rational do not avoid coercion in rich comparisons, they just try to avoid it for most common cases.

Do you have an example of a Parent where we want to avoid coercions?

comment:61 in reply to: ↑ 60 ; follow-up: Changed 7 years ago by jdemeyer

Replying to vdelecroix:

Right, but _richcmp will call _richcmp_/_cmp_. In Integers (and Rationals after #18304) there is also this _cmp_ implemented and if everything else failed in __richcmp__ there is precisely Element._richcmp(self, other, op).

So even if you implement a subset of ZZ/QQ as a new parent (let say the additive group 3 ZZ or the ring ZZ[1/p]) then it would be fine with comparisons with integers/rationals as soon as you define a coercion 3 ZZ -> ZZ/ZZ[1/p] -> QQ. So my mistake, the situation for ZZ and QQ is a bit different: Integer/Rational do not avoid coercion in rich comparisons, they just try to avoid it for most common cases.

I guess you're right. It's just much less obvious (as _cmp_ is used after coercion, not __richcmp__).

Do you have an example of a Parent where we want to avoid coercions?

See src/sage/numerical/linear_functions.pyx which deals with "symbolic" linear inequalities. Here, (a <= b) <= c should construct the symbolic inequality a <= b <= c.

comment:62 follow-up: Changed 7 years ago by vdelecroix

I would really like the following in Element._richcmp_

  • src/sage/structure/element.pyx

    diff --git a/src/sage/structure/element.pyx b/src/sage/structure/element.pyx
    index 6981135..12cb669 100644
    a b cdef class Element(SageObject): 
    10471047            ...
    10481048            NotImplementedError: comparison not implemented for <type 'sage.structure.element.Element'>
    10491049        """
    10501050        # Obvious case
    10511051        if left is right:
    10521052            return rich_to_bool(op, 0)
    10531053
    10541054        cdef int c
     1055        if op == Py_NE:
     1056            try:
     1057                left_eq = left.__eq__
     1058            except AttributeError:
     1059                pass
     1060            else:
     1061                if isinstance(left_eq, MethodType):
     1062                    return False if left_eq(right) else True
     1063
    10551064        try:
    10561065            c = left._cmp_(right)
    10571066        except NotImplementedError:

that would allow element implementing __eq__ to benefit of __ne__ for free. What do you think? Perhaps not for this ticket though (thinking about #18305). It is a bit dangerous since __eq__ would not go through coercions but __ne__ would.

comment:63 in reply to: ↑ 61 Changed 7 years ago by vdelecroix

Replying to jdemeyer:

Replying to vdelecroix:

Right, but _richcmp will call _richcmp_/_cmp_. In Integers (and Rationals after #18304) there is also this _cmp_ implemented and if everything else failed in __richcmp__ there is precisely Element._richcmp(self, other, op).

So even if you implement a subset of ZZ/QQ as a new parent (let say the additive group 3 ZZ or the ring ZZ[1/p]) then it would be fine with comparisons with integers/rationals as soon as you define a coercion 3 ZZ -> ZZ/ZZ[1/p] -> QQ. So my mistake, the situation for ZZ and QQ is a bit different: Integer/Rational do not avoid coercion in rich comparisons, they just try to avoid it for most common cases.

I guess you're right. It's just much less obvious (as _cmp_ is used after coercion, not __richcmp__).

Do you have an example of a Parent where we want to avoid coercions?

See src/sage/numerical/linear_functions.pyx which deals with "symbolic" linear inequalities. Here, (a <= b) <= c should construct the symbolic inequality a <= b <= c.

I see. Could you add this one to the documentation of Element._richcmp then?

comment:64 in reply to: ↑ 62 Changed 7 years ago by jdemeyer

Replying to vdelecroix:

that would allow element implementing __eq__ to benefit of __ne__ for free. What do you think? Perhaps not for this ticket though (thinking about #18305). It is a bit dangerous since __eq__ would not go through coercions but __ne__ would.

I don't like it so much. I think a Python class can easily do

def __ne__(self, other):
    return not (self == other)

And then you're indeed bringing coercion in the story and you're assuming that rich comparisons return True or False and not something symbolic.

comment:65 Changed 7 years ago by git

  • Commit changed from 04570b3dfd194fea8c2437c8717c3dfb8704b4af to 3976f2cc4ec59c9f978d9d02c7f697c81025c4ef

Branch pushed to git repo; I updated commit sha1. New commits:

3976f2cAdd pointers for special uses of __richcmp__

comment:66 Changed 7 years ago by vdelecroix

I am running long tests. If these are good, this is good for me.

Vincent

comment:67 Changed 7 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to positive_review

All tests passed!

comment:68 Changed 7 years ago by jdemeyer

Great, thanks!

comment:69 Changed 7 years ago by jdemeyer

Another follow-up: #18322

comment:70 Changed 7 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/17890 to 3976f2cc4ec59c9f978d9d02c7f697c81025c4ef
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.