Opened 5 years ago
Closed 5 years ago
#21176 closed enhancement (fixed)
getting rid of some more cmp() in pyx files
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage7.4 
Component:  python3  Keywords:  
Cc:  jdemeyer, tscrim  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  38cce3d (Commits, GitHub, GitLab)  Commit:  38cce3dcd24ce9b8096fda496e0bc2babbf7b293 
Dependencies:  Stopgaps: 
Description (last modified by )
as small steps towards py3
here is another bunch of cmp removed
method: try to compile sage with python3 and solve one failure after the other.
This should help to see what kinds of problems and solutions we will meet about cmp.
Change History (13)
comment:1 Changed 5 years ago by
 Branch set to public/21176
 Commit set to 4b3e415280e4975d595f77aebe03b85c034425f8
comment:2 Changed 5 years ago by
 Commit changed from 4b3e415280e4975d595f77aebe03b85c034425f8 to c7c7b0cbcc90ad04f872507418a3fb3c0ed2db73
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
c7c7b0c  remove cmp in morphism, binary_code, data_structures_pyx.pxi, codecan, etc

comment:3 Changed 5 years ago by
 Cc jdemeyer tscrim added
 Description modified (diff)
 Status changed from new to needs_review
 Type changed from PLEASE CHANGE to enhancement
comment:4 Changed 5 years ago by
For the record, I think that getting rid of __cmp__
is more important than getting rid of cmp
. The reason is that we could always just write a cmp()
function but we cannot easily deal with __cmp__
.
comment:5 Changed 5 years ago by
bot is green
comment:6 Changed 5 years ago by
About src/sage/categories/morphism.pyx
:
You are changing code of the form
c = cmp(x.A, y.A) if c: return c c = cmp(x.B, y.B) if c: return c c = cmp(x.C, y.C) return c
to
t = (x.A, x.B, x.C) u = (y.A, y.B, y.C) return richcmp(t, u, op)
The latter is less efficient because it does not "lazily" compute the .B
and .C
attributes.
I think that more efficient code should be written. Here is an idea:
# Helper method in sage_object.pxd (based on #21128 to avoid conflicts) cpdef inline richcmp_not_equal(x, y, int op): """ Like ``richcmp(x, y, op)`` but where it is known that `x` is not equal to `y`. """ if op == Py_EQ: return False if op == Py_NE: return True return PyObject_RichCompare(x, y, op) # Actual code in _richcmp_ if x.A != y.A: return richcmp_not_equal(x.A, y.A, op) if x.B != y.B: return richcmp_not_equal(x.B, y.B, op) return PyObject_RichCompare(x.C, y.C, op)
comment:7 Changed 5 years ago by
In fact, I think this _cmp_
in morphism is probably never used anywhere. The current code in the branch is broken (self.domain is missing parentheses), and yet all doctests pass !
About the general issue you ask, what about having a method
richcmp_lazy_for_list(self, other, list_of_functions, op)
where list of functions would be used as a "lazykey" for comparison.
In the example appearing in morphism, we would use
richcmp_lazy_for_list(self, other, [lambda x: x.domain(), lambda x: x.codomain(), lambda x: tuple(x(gen) for gen in x.domain().gens())], op)
This richcmp_lazy_for_list
would evaluate step by step, using as few functions as possible.
We are really hit hard here by the rather dubious preference in python3 for sorting keys rather than sorting functions. I think we need some kind of lazykey mechanism.
comment:8 Changed 5 years ago by
My main goal was to improve performance. I don't know whether complicated constructions using lists of lambda
s will meet that goal.
comment:9 Changed 5 years ago by
 Status changed from needs_review to needs_work
comment:10 Changed 5 years ago by
 Commit changed from c7c7b0cbcc90ad04f872507418a3fb3c0ed2db73 to 38cce3dcd24ce9b8096fda496e0bc2babbf7b293
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
38cce3d  remove cmp in binary_code, codecan, etc

comment:11 Changed 5 years ago by
 Reviewers set to Jeroen Demeyer
 Status changed from needs_work to positive_review
I removed the changes in src/sage/categories/morphism.pyx
and src/sage/groups/perm_gps/partn_ref/data_structures_pyx.pxi
. For the latter, I don't think your branch was equivalent with the old code (at least it was not obvious to me).
comment:12 Changed 5 years ago by
Thanks.
For morphism, I understand and agree.
The other one was good I think.
comment:13 Changed 5 years ago by
 Branch changed from public/21176 to 38cce3dcd24ce9b8096fda496e0bc2babbf7b293
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
cmp in morphism.pyx
fix
cmp in binary_code
cmp in data_structures_pyx.pxi
fix
cmp in codecan
cmp in crystals of letters
using a sign function
cmp in dancing_links