Opened 7 years ago
Closed 7 years ago
#18305 closed enhancement (fixed)
Element comparison for Python classes
Reported by: | vdelecroix | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.9 |
Component: | coercion | Keywords: | |
Cc: | jdemeyer, jpflori | Merged in: | |
Authors: | Vincent Delecroix | Reviewers: | Jeroen Demeyer |
Report Upstream: | N/A | Work issues: | |
Branch: | 0ee1f3a (Commits, GitHub, GitLab) | Commit: | 0ee1f3a96c0db1834efe22def0c2556b9e83355b |
Dependencies: | Stopgaps: |
Description (last modified by )
In #17890 the comparison of elements (i.e. Element.__cmp__
) and rich comparison of elements (i.e. Element.__richcmp__
) have been simplified.
1 .If coercion has to be involved then the Python elements should implement:
_richcmp_(left, right, op)
for rich comparisons (i.e. operators<
,<=
,==
,!=
,>=
and>=
)_cmp_(left, right)
for comparisons (i.e. when callingcmp(x,y)
) In these two methods, it can be assumed that both argumentsleft
andright
are subclasses ofElement
with the same parent. If_cmp_
makes sense it is possible to implement it only. If an element wants to implement==
and!=
only, then it should just raise aNotImplementedError
for other operators in_richcmp_(left,right,op)
.
2 . If comparison needs to not avoid coercion, then:
- rich comparison is possible through
__eq__
,__ne__
,__lt__
, etc (and all of these should be implemented) - comparison is possible via
__cmp__
With the current branch:
- make everything needed for rich comparisons available from Python: the branch adds
py_rich_to_bool
andop_EQ
,op_NE
,op_LT
,op_LE
,op_GE
,op_GT
insage.structure.sage_object
- six elements are modified to use either
_richcmp_
or_cmp_
instead of__eq__
/__ne__
/__lt__
, etc (and #18303 takes care ofsage.rings.qqbar
)
Potentially to discuss:
- decide what should we do with respect to
x < y
when this makes no sense (some elements do raise aTypeError
) - remove the test in the default
Element._cmp_
since an element implementing__cmp__
wants to avoid coercion
Change History (43)
comment:1 follow-up: ↓ 2 Changed 7 years ago by
comment:2 in reply to: ↑ 1 Changed 7 years ago by
Replying to jdemeyer:
I think in general, rich comparisons (both Cython's
__richcmp__
and Python's__eq__
and friends) should be discouraged in favour of_cmp_
, because the latter needs coercion only once. Only for more complicated types where rich comparisons cannot be implemented in terms of__cmp__
(intervals for example) should rich comparisons be used.Do you agree?
Partly. I agree that if _cmp_
is doable and cheap then it is preferable. But
- consider lists (or words or sequences), you have a cheap test to check whether they are different: comparing the length. So in that case, you might want to use it before comparing items one by one for getting the lexicographic ordering.
- there are elements where equality makes sense but not comparison.
_richcmp_
can handle that but not_cmp_
. In such case I would implement_richcmp_
and raise an error if the operator is notPy_EQ
orPy_NE
.
And I am not sure that I understand why _cmp_
needs less coercion than _richcmp_
.
comment:3 Changed 7 years ago by
- Description modified (diff)
Another example: embedded number fields in RR
. If you want that rich comparisons coincide with the order on RR
then ==
and !=
are cheap but not <
, <=
, >
and >=
.
Do you agree with the new description?
comment:4 follow-up: ↓ 12 Changed 7 years ago by
- Branch set to u/vdelecroix/18305
- Commit set to 7749fe882a88d0978413e9c58b9d2660e0de23a6
- Dependencies changed from #17890, #18303 to #17890
- Description modified (diff)
I was able to modify six Python elements to use coercion for their comparisons. The code is simplified.
In order to do that, I had to modify slightly the implementation of _richcmp
to not catch a TypeError
when it calls _richcmp_
. We should precise what elements should do when ==
and !=
makes sense but not <
, <=
, >=
, >
. Is TypeError
appropriate?
Vincent
Last 10 new commits:
04570b3 | Fix bad doctest in etaproducts
|
3976f2c | Add pointers for special uses of __richcmp__
|
ce7e4f1 | merge u/jdemeyer/ticket/17890 in Sage-6.7.beta3
|
203820b | Trac 18305: py_rich_to_bool(_sgn) and operators
|
c6d3e23 | Trac 18305: use `_cmp_` for alternating sign matrices
|
f3e46e3 | Trac 18305: use _cmp_ for ideals of finite dimensional algebras
|
b0c39b0 | Trac 18305: _richcmp_ for indexed monoids and groups
|
220a140 | Trac 18305: _richcmp_ for Newton polygons
|
042b6fe | Trac 18305: modify Element._richcmp so that _richcmp_ can raise errors
|
7749fe8 | Trac 18305: _richcmp_ for `finite dimensional algebra elements
|
comment:5 Changed 7 years ago by
- Commit changed from 7749fe882a88d0978413e9c58b9d2660e0de23a6 to 81be759f0e28dc13aa17e2f7df72bee0e729ee8f
Branch pushed to git repo; I updated commit sha1. New commits:
81be759 | Trac 18305: update comments in sage.structure.element
|
comment:6 Changed 7 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:7 follow-up: ↓ 8 Changed 7 years ago by
comment:8 in reply to: ↑ 7 Changed 7 years ago by
Replying to jdemeyer:
I plan to have a look at this when #17890 is merged. I would also like to have some feedback on #18322, since both this ticket and #18322 change
Element
.
Perhaps I should set #18322 as a dependency? I do not think that there will be any conflict since nothing in Sage explicitely uses cmp
(except doctests).
Vincent
comment:9 Changed 7 years ago by
- Commit changed from 81be759f0e28dc13aa17e2f7df72bee0e729ee8f to df2b501daa2d5b9ebde44d8e698f3d7f51228299
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
b70f3f2 | Trac 18305: py_rich_to_bool(_sgn) and operators
|
2edd0d4 | Trac 18305: use `_cmp_` for alternating sign matrices
|
240ba64 | Trac 18305: use _cmp_ for ideals of finite dimensional algebras
|
7bccbcc | Trac 18305: _richcmp_ for indexed monoids and groups
|
4663b80 | Trac 18305: _richcmp_ for Newton polygons
|
06e05f2 | Trac 18305: modify Element._richcmp so that _richcmp_ can raise errors
|
9ad4257 | Trac 18305: _richcmp_ for `finite dimensional algebra elements
|
df2b501 | Trac 18305: update comments in sage.structure.element
|
comment:10 Changed 7 years ago by
Rebase on sage-6.7.beta4
comment:11 Changed 7 years ago by
- Dependencies #17890 deleted
comment:12 in reply to: ↑ 4 ; follow-up: ↓ 13 Changed 7 years ago by
Replying to vdelecroix:
042b6fe Trac 18305: modify Element._richcmp so that _richcmp_ can raise errors
I moved this to #18322 since I think it fits better there.
comment:13 in reply to: ↑ 12 Changed 7 years ago by
- Dependencies set to #18322
- Status changed from needs_review to needs_work
Replying to jdemeyer:
Replying to vdelecroix:
042b6fe Trac 18305: modify Element._richcmp so that _richcmp_ can raise errors
I moved this to #18322 since I think it fits better there.
Good to me.
comment:14 Changed 7 years ago by
About this ticket: I don't really like that you're making major unrelated changes, for example (but not only) in alternating sign matrices. Do you really need that? Could those changes be moved to a different ticket?
comment:15 Changed 7 years ago by
- Commit changed from df2b501daa2d5b9ebde44d8e698f3d7f51228299 to 0259eb7814ac159722db566fcd52c21e040cb233
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
bb98fd0 | _cmp should try _richcmp_ if _cmp_ failed
|
d826520 | Use LazyFormat for _cmp_ exception
|
ac9ca1c | Update comment
|
89d1b0b | Trac 18305: py_rich_to_bool(_sgn) and operators
|
1078589 | Trac 18305: use _cmp_ for ideals of finite dimensional algebras
|
7f6c59f | Trac 18305: _richcmp_ for indexed monoids and groups
|
0fb7c8e | Trac 18305: _richcmp_ for Newton polygons
|
51abce8 | Trac 18305: _richcmp_ for `finite dimensional algebra elements
|
0259eb7 | Trac 18305: update comments in sage.structure.element
|
comment:16 Changed 7 years ago by
comment:17 Changed 7 years ago by
- Status changed from needs_work to needs_review
comment:18 Changed 7 years ago by
- Cc jpflori added
comment:19 Changed 7 years ago by
I think having both py_rich_to_bool_sgn
and py_rich_to_bool
in Python is overkill. Since these are Python functions anyway, the overhead of the _sgn
variant can be ignored.
And I don't like most of your comment changes to src/sage/structure/element.pyx
: if you implement just _richcmp_
, then you don't need __cmp__
, not even for cmp()
. Why did you remove or if that gives better performance
. And the and are not identical (i.e. self is not other).
addition is also wrong I think.
comment:20 Changed 7 years ago by
Also, there is no need for the int
in
def py_rich_to_bool_sgn(int op, int c)
(if we ever change the prototype of rich_to_bool_sgn
, there is additional work for no reason)
comment:21 Changed 7 years ago by
- Commit changed from 0259eb7814ac159722db566fcd52c21e040cb233 to 17ed5499a2113637034ffc8ef2210f949896e5c6
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
c78d7ce | Trac 18305: py_rich_to_bool(_sgn) and operators
|
1bc5ee9 | Trac 18305: use _cmp_ for ideals of finite dimensional algebras
|
71409ec | Trac 18305: _richcmp_ for indexed monoids and groups
|
fc3b7a1 | Trac 18305: _richcmp_ for Newton polygons
|
17e7496 | Trac 18305: _richcmp_ for `finite dimensional algebra elements
|
17ed549 | Trac #18305: comments about Python comparisons
|
comment:22 Changed 7 years ago by
- Dependencies #18322 deleted
- Description modified (diff)
Rebased on 6.8.beta0.
I took care of your comments.
Vincent
comment:23 Changed 7 years ago by
- Status changed from needs_review to needs_work
does not apply, needs rebase
comment:24 Changed 7 years ago by
- Commit changed from 17ed5499a2113637034ffc8ef2210f949896e5c6 to d768d2524b35e70345bf3b94392dd3e44ade9376
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
522d61f | Trac 18305: py_rich_to_bool(_sgn) and operators
|
6089576 | Trac 18305: use _cmp_ for ideals of finite dimensional algebras
|
2cc324c | Trac 18305: _richcmp_ for indexed monoids and groups
|
88fd79e | Trac 18305: _richcmp_ for Newton polygons
|
690b959 | Trac 18305: _richcmp_ for `finite dimensional algebra elements
|
d768d25 | Trac #18305: comments about Python comparisons
|
comment:25 Changed 7 years ago by
- Milestone changed from sage-6.7 to sage-6.9
- Status changed from needs_work to needs_review
rebased on sage-6.9.beta1
comment:26 Changed 7 years ago by
See also #19108 which solves the same problem in a different (more complicated) way.
comment:27 follow-up: ↓ 30 Changed 7 years ago by
Can you split off the changes to src/sage/algebras
to a different ticket?
comment:28 Changed 7 years ago by
Replace
If you need to call this function from Cython prefer
by
Do not use this function from Cython. Instead, call
comment:29 Changed 7 years ago by
- Commit changed from d768d2524b35e70345bf3b94392dd3e44ade9376 to 3f6ea84eb645f1e29426bedc7763c9f31997867b
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
a9283cb | Trac 18305: py_rich_to_bool(_sgn) and operators
|
e8b8182 | Trac 18305: _richcmp_ for indexed monoids and groups
|
950f3ec | Trac 18305: _richcmp_ for Newton polygons
|
490de42 | Trac 18305: comments about Python comparisons
|
3f6ea84 | Trac 18305: rephrased sentence in doc
|
comment:30 in reply to: ↑ 27 Changed 7 years ago by
comment:31 Changed 7 years ago by
- Status changed from needs_review to needs_work
This is not good, you're adding back stuff which was removed:
-
src/sage/structure/element.pyx
diff --git a/src/sage/structure/element.pyx b/src/sage/structure/element.pyx index ede0157..4208a50 100644
a b cdef class Element(SageObject): 1043 1043 return rich_to_bool(op, -1) 1044 1044 1045 1045 #################################################################### 1046 # For a Cython class, you must define either _cmp_ (if your subclass 1047 # is totally ordered), _richcmp_ (if your subclass is partially 1048 # ordered), or both (if your class has both a total order and a 1049 # partial order, or if implementing both gives better performance). 1046 # For a derived Cython class, you **must** put the __richcmp__ 1047 # method below in your subclasses, in order for it to take 1048 # advantage of the above generic comparison code. If you want to use cmp() 1049 # but did not implement a rich comparison, then you must copy paste the 1050 # __cmp__ method below. 1051 # 1052 # In a Cython or a Python class, you must define either _cmp_ 1053 # (if your subclass is totally ordered), _richcmp_ (if your subclass 1054 # is partially ordered), or both (if your class has both a total order 1055 # and a partial order, or if that gives better performance). 1050 1056 # 1051 1057 # Rich comparisons (like a < b) will default to using _richcmp_, 1052 1058 # three-way comparisons (like cmp(a,b)) will default to using
comment:32 Changed 7 years ago by
Py_EQ
should be op_EQ
here:
INPUT: - ``op`` -- a rich comparison operation (e.g. ``Py_EQ``)
c
can be any integer, not only -1, 0 or 1:
- ``c`` -- the result of an ordinary comparison: -1, 0 or 1.
No trailing slash needed:
sage: from sage.structure.sage_object import (py_rich_to_bool, \ ....: op_EQ, op_NE, op_LT, op_LE, op_GT, op_GE)
comment:33 Changed 7 years ago by
Please keep the doctests for IndexedMonoidElement.__ne__
like you did for the other comparisons.
comment:34 Changed 7 years ago by
$ pyflakes src/sage/geometry/newton_polygon.py src/sage/geometry/newton_polygon.py:20: 'op_GT' imported but unused src/sage/geometry/newton_polygon.py:20: 'py_rich_to_bool' imported but unused
comment:35 Changed 7 years ago by
- Commit changed from 3f6ea84eb645f1e29426bedc7763c9f31997867b to 0ee1f3a96c0db1834efe22def0c2556b9e83355b
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
3959aad | Trac 18305: py_rich_to_bool(_sgn) and operators
|
6be9889 | Trac 18305: _richcmp_ for indexed monoids and groups
|
f3450e0 | Trac 18305: _richcmp_ for Newton polygons
|
0ee1f3a | Trac 18305: comments about Python comparisons
|
comment:36 follow-up: ↓ 38 Changed 7 years ago by
Hello Jeroen,
I rebased on the latest beta taking care of all your remarks.
Vincent
comment:37 Changed 7 years ago by
- Reviewers set to Jeroen Demeyer
- Status changed from needs_work to needs_review
comment:38 in reply to: ↑ 36 Changed 7 years ago by
Replying to vdelecroix:
I rebased on the latest beta taking care of all your remarks.
General comment: you should not rebase a branch which is being reviewed. For example, it is difficult for me to see what you changed since last time, which essentially forces me to re-review everything again.
comment:39 Changed 7 years ago by
If you tell me, I can move back my changes on top of the previous commits. Sorry.
comment:40 Changed 7 years ago by
- Status changed from needs_review to positive_review
comment:41 follow-up: ↓ 42 Changed 7 years ago by
Thanks Jeroen. And again, I am sorry for the too early rebase.
Vincent
comment:42 in reply to: ↑ 41 Changed 7 years ago by
Replying to vdelecroix:
Thanks Jeroen. And again, I am sorry for the too early rebase.
Why "too early"? I don't think a ticket under review should be rebased at all.
Rebasing before review is fine for me.
comment:43 Changed 7 years ago by
- Branch changed from u/vdelecroix/18305 to 0ee1f3a96c0db1834efe22def0c2556b9e83355b
- Resolution set to fixed
- Status changed from positive_review to closed
I think in general, rich comparisons (both Cython's
__richcmp__
and Python's__eq__
and friends) should be discouraged in favour of_cmp_
, because the latter needs coercion only once. Only for more complicated types where rich comparisons cannot be implemented in terms of__cmp__
(intervals for example) should rich comparisons be used.Do you agree?