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:  sage6.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: 
Description (last modified by )
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
 Description modified (diff)
 Summary changed from Remove _cmp_c_impl to Remove _(rich)cmp_c_impl
comment:2 Changed 7 years ago by
 Description modified (diff)
comment:3 Changed 7 years ago by
 Description modified (diff)
comment:4 Changed 7 years ago by
 Branch set to u/jdemeyer/ticket/17890
comment:5 Changed 7 years ago by
 Commit set to a830cc93c6bb07565db709bfe03f8acc62034dbb
 Description modified (diff)
comment:6 followup: ↓ 7 Changed 7 years ago by
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
Replying to vdelecroix:
If it is slower this is very bad. Do you know why?
Yes, see https://groups.google.com/forum/#!topic/sagedevel/AXYk1uEfuPE
comment:8 Changed 7 years ago by
Related: #10130.
comment:9 Changed 7 years ago by
 Commit changed from a830cc93c6bb07565db709bfe03f8acc62034dbb to 25e517fc735d15ba07c919f7a65103cf04acd2cf
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
25e517f  Remove _cmp_c_impl and _richcmp_c_impl

comment:10 Changed 7 years ago by
 Description modified (diff)
comment:11 Changed 7 years ago by
 Dependencies set to #17862
comment:12 Changed 7 years ago by
 Commit changed from 25e517fc735d15ba07c919f7a65103cf04acd2cf to 41f7cd14911ed5fde23731c2c78f235e481258e6
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
41f7cd1  Remove _cmp_c_impl and _richcmp_c_impl

comment:13 Changed 7 years ago by
 Status changed from new to needs_review
comment:14 Changed 7 years ago by
See also #16537
comment:15 Changed 7 years ago by
 Commit changed from 41f7cd14911ed5fde23731c2c78f235e481258e6 to d26954c093a671139347c65ee7772b79e9a02783
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
d26954c  Remove _cmp_c_impl and _richcmp_c_impl

comment:16 Changed 7 years ago by
 Dependencies #17862 deleted
comment:17 Changed 7 years ago by
 Commit changed from d26954c093a671139347c65ee7772b79e9a02783 to ea13fd01a8a37ce5d6d42e58d6c5f047250d3f38
Branch pushed to git repo; I updated commit sha1. New commits:
ea13fd0  Merge tag '6.6.beta5' into t/17890/ticket/17890

comment:18 Changed 7 years ago by
 Cc dkrenn added
comment:19 Changed 7 years ago by
 Commit changed from ea13fd01a8a37ce5d6d42e58d6c5f047250d3f38 to a7cbb88852ceefff65e421e75c0479bcd554ce26
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
a7cbb88  Remove _cmp_c_impl and _richcmp_c_impl

comment:20 followups: ↓ 21 ↓ 22 ↓ 23 Changed 7 years ago by
 Status changed from needs_review to needs_info
sage.structure.element
 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
 implement
 What about putting
# Obvious case if left is right: return _rich_to_bool(op, 0)
directly in__richcmp__
?
sage.structure.parent
 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?
 Why not using
_rich_to_bool
inparent.pyx
instead ofif 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 methodcdef _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
Replying to vdelecroix:
sage.structure.element
 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.
comment:22 in reply to: ↑ 20 Changed 7 years ago by
Replying to vdelecroix:
 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
Replying to vdelecroix:
 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
 Commit changed from a7cbb88852ceefff65e421e75c0479bcd554ce26 to 1ad339bf0c8ffe75d23ddaa6d896121b8c96636d
comment:25 Changed 7 years ago by
 Commit changed from 1ad339bf0c8ffe75d23ddaa6d896121b8c96636d to 17bd067153ad40ced061147390687f514a7f7cba
Branch pushed to git repo; I updated commit sha1. New commits:
17bd067  Merge tag '6.7.beta2' into t/17890/ticket/17890

comment:26 Changed 7 years ago by
 Status changed from needs_info to needs_review
OK, I improved rich_to_bool
a lot.
comment:27 followup: ↓ 30 Changed 7 years ago by
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)
witha
andb
between two parentsA
andB
and a coercionB > A
thenElement.__richcmp__
is called instead ofElement.__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 followups: ↓ 29 ↓ 32 Changed 7 years ago by
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
Replying to vdelecroix:
Moreover if you redefine
__cmp__
in your Element class thenElement.__cmp__
is not called! So the test inElement._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 ; followup: ↓ 34 Changed 7 years ago by
Replying to vdelecroix:
 comparison involving coercion is wrong: if you call
cmp(a,b)
witha
andb
between two parentsA
andB
and a coercionB > A
thenElement.__richcmp__
is called instead ofElement.__cmp__
.Actually, this is not only a bug in Python
Bug or feature?
comment:31 Changed 7 years ago by
And implementing _richcmp_
instead of __cmp__
in AlgebraicReal
I got an incredible speedup (see #18303).
comment:32 in reply to: ↑ 28 ; followup: ↓ 33 Changed 7 years ago by
Replying to vdelecroix:
Moreover if you redefine
__cmp__
in your Element class thenElement.__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 ; followup: ↓ 36 Changed 7 years ago by
Replying to jdemeyer:
Replying to vdelecroix:
Moreover if you redefine
__cmp__
in your Element class thenElement.__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 ; followup: ↓ 35 Changed 7 years ago by
Replying to jdemeyer:
Replying to vdelecroix:
 comparison involving coercion is wrong: if you call
cmp(a,b)
witha
andb
between two parentsA
andB
and a coercionB > A
thenElement.__richcmp__
is called instead ofElement.__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 ; followup: ↓ 38 Changed 7 years ago by
Replying to vdelecroix:
Call it whatever you want:
cmp(a1,a2)
(same type) calls__cmp__
butcmp(a,b)
(different types) does not and use__richcmp__
instead. The consequence is that you might perform three times the coercion while callingcmp
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 ; followup: ↓ 37 Changed 7 years ago by
Replying to vdelecroix:
Replying to jdemeyer:
Replying to vdelecroix:
Moreover if you redefine
__cmp__
in your Element class thenElement.__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 ; followup: ↓ 41 Changed 7 years ago by
Replying to jdemeyer:
Replying to vdelecroix:
Replying to jdemeyer:
Replying to vdelecroix:
Moreover if you redefine
__cmp__
in your Element class thenElement.__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 redefinefoo
in your Element class thenElement.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
Replying to jdemeyer:
Replying to vdelecroix:
Call it whatever you want:
cmp(a1,a2)
(same type) calls__cmp__
butcmp(a,b)
(different types) does not and use__richcmp__
instead. The consequence is that you might perform three times the coercion while callingcmp
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 followup: ↓ 44 Changed 7 years ago by
 Status changed from needs_review to needs_work
 For Python classes, I guess that one needs to implement
_cmp_
and/or_richcmp_
. This must be precised in the doc ofElement/Parent
. For this reason, we need apy_rich_to_bool
available at the level of Python (as well as the operatorsPy_EQ
,Py_NE
, etc).
 I think we should avoid implenting
__eq__
,__ne__
because one can do with_richcmp_
that would take care of coercions. What do you think?
 There should be a mention of the Python issue that
cmp(a,b)
witha
andb
of different types does not call_cmp_
(or__cmp__
)
comment:40 followup: ↓ 43 Changed 7 years ago by
 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 inInteger._cmp_
, you could optimize the sign check as well (either with your version or the one above)  why the input is
long c
and notint c
? the output ofmpz_cmp
is anint
for example.
 I found the following more readable
comment:41 in reply to: ↑ 37 Changed 7 years ago by
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
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 ; followup: ↓ 47 Changed 7 years ago by
Replying to vdelecroix:
 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 notint c
? the output ofmpz_cmp
is anint
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 ; followup: ↓ 45 Changed 7 years ago by
Replying to vdelecroix:
 For Python classes, I guess that one needs to implement
_cmp_
and/or_richcmp_
. This must be precised in the doc ofElement/Parent
. For this reason, we need apy_rich_to_bool
available at the level of Python (as well as the operatorsPy_EQ
,Py_NE
, etc).
 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
Replying to jdemeyer:
Replying to vdelecroix:
 For Python classes, I guess that one needs to implement
_cmp_
and/or_richcmp_
. This must be precised in the doc ofElement/Parent
. For this reason, we need apy_rich_to_bool
available at the level of Python (as well as the operatorsPy_EQ
,Py_NE
, etc).
 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 to tickets depending on it ;) I opened #18305
Vincent
comment:46 Changed 7 years ago by
For the documentation, I opened #18306.
comment:47 in reply to: ↑ 43 Changed 7 years ago by
Replying to jdemeyer:
It's an
inline
function, so it won't matter much for performance.long
is more general thanint
, so if there is ever a use case for along
, 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
 Commit changed from 17bd067153ad40ced061147390687f514a7f7cba to 313a400146b0fb19d80c31117d44ce735041d4ec
Branch pushed to git repo; I updated commit sha1. New commits:
313a400  Optimize rich_to_bool_sgn

comment:49 Changed 7 years ago by
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:
313a400  Optimize rich_to_bool_sgn

comment:50 Changed 7 years ago by
 Status changed from needs_work to needs_review
comment:51 followup: ↓ 52 Changed 7 years ago by
 Status changed from needs_review to needs_info
 implementation of
_(rich)cmp
: Why in
_(rich)cmp
it starts with the casenot have_same_parent_c(left,right)
instead ofhave_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 withleft, right = coercion_model.canonical_coercion(left, right)
. As far as I see, there is no need of these extra variables.
 Why in
 Could you add some minimal documentation to
_(rich)cmp
and_(rich)cmp_
likedef _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.
comment:52 in reply to: ↑ 51 Changed 7 years ago by
Replying to vdelecroix:
 implementation of
_(rich)cmp
:
 Why in
_(rich)cmp
it starts with the casenot have_same_parent_c(left,right)
instead ofhave_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 withleft, 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
 Commit changed from 313a400146b0fb19d80c31117d44ce735041d4ec to 0d1e049f807a1c34b00cf0e58ee65cc7734d1ad1
comment:54 Changed 7 years ago by
 Milestone changed from sage6.6 to sage6.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 followup: ↓ 56 Changed 7 years ago by
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 ; followup: ↓ 60 Changed 7 years ago by
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 subrings: it is never the common parent of two different parents.
comment:57 Changed 7 years ago by
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
 Commit changed from 0d1e049f807a1c34b00cf0e58ee65cc7734d1ad1 to 39273f1aacfdd307d20b729ae732e349438a8422
Branch pushed to git repo; I updated commit sha1. New commits:
39273f1  Fix doctest formatting

comment:59 Changed 7 years ago by
 Commit changed from 39273f1aacfdd307d20b729ae732e349438a8422 to 04570b3dfd194fea8c2437c8717c3dfb8704b4af
Branch pushed to git repo; I updated commit sha1. New commits:
04570b3  Fix bad doctest in etaproducts

comment:60 in reply to: ↑ 56 ; followup: ↓ 61 Changed 7 years ago by
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 forInteger
is thatZZ
has no subrings: 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 ; followup: ↓ 63 Changed 7 years ago by
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 preciselyElement._richcmp(self, other, op)
.So even if you implement a subset of
ZZ
/3 ZZ
or the ringZZ[1/p]
) then it would be fine with comparisons with integers/rationals as soon as you define a coercion3 ZZ > ZZ
/ZZ[1/p] > QQ
. So my mistake, the situation forZZ
andInteger/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 followup: ↓ 64 Changed 7 years ago by
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): 1047 1047 ... 1048 1048 NotImplementedError: comparison not implemented for <type 'sage.structure.element.Element'> 1049 1049 """ 1050 1050 # Obvious case 1051 1051 if left is right: 1052 1052 return rich_to_bool(op, 0) 1053 1053 1054 1054 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 1055 1064 try: 1056 1065 c = left._cmp_(right) 1057 1066 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
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 preciselyElement._richcmp(self, other, op)
.So even if you implement a subset of
ZZ
/3 ZZ
or the ringZZ[1/p]
) then it would be fine with comparisons with integers/rationals as soon as you define a coercion3 ZZ > ZZ
/ZZ[1/p] > QQ
. So my mistake, the situation forZZ
andInteger/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 inequalitya <= 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
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
 Commit changed from 04570b3dfd194fea8c2437c8717c3dfb8704b4af to 3976f2cc4ec59c9f978d9d02c7f697c81025c4ef
Branch pushed to git repo; I updated commit sha1. New commits:
3976f2c  Add pointers for special uses of __richcmp__

comment:66 Changed 7 years ago by
I am running long tests. If these are good, this is good for me.
Vincent
comment:67 Changed 7 years ago by
 Reviewers set to Vincent Delecroix
 Status changed from needs_review to positive_review
All tests passed!
comment:68 Changed 7 years ago by
Great, thanks!
comment:69 Changed 7 years ago by
Another followup: #18322
comment:70 Changed 7 years ago by
 Branch changed from u/jdemeyer/ticket/17890 to 3976f2cc4ec59c9f978d9d02c7f697c81025c4ef
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Remove use of PY_IS_NUMERIC
Move py_scalar_to_element to coerce.pyx
trac #17862 (review): doc typo
Merge tag '6.6.beta2' into HEAD
Remove _cmp_c_impl and _richcmp_c_impl