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: sage-7.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:

Status badges

Description (last modified by chapoton)

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 chapoton

  • Branch set to public/21176
  • Commit set to 4b3e415280e4975d595f77aebe03b85c034425f8

New commits:

74b61cacmp in morphism.pyx
49c9122fix
d69bb0fcmp in binary_code
5099c4dcmp in data_structures_pyx.pxi
0f559c8fix
2933e53cmp in codecan
94dd42ccmp in crystals of letters
eea60ddusing a sign function
4b3e415cmp in dancing_links

comment:2 Changed 5 years ago by git

  • Commit changed from 4b3e415280e4975d595f77aebe03b85c034425f8 to c7c7b0cbcc90ad04f872507418a3fb3c0ed2db73

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

c7c7b0cremove cmp in morphism, binary_code, data_structures_pyx.pxi, codecan, etc

comment:3 Changed 5 years ago by chapoton

  • 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 jdemeyer

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 chapoton

bot is green

comment:6 Changed 5 years ago by jdemeyer

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)
Last edited 5 years ago by jdemeyer (previous) (diff)

comment:7 Changed 5 years ago by chapoton

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 "lazy-key" 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 lazy-key mechanism.

comment:8 Changed 5 years ago by jdemeyer

My main goal was to improve performance. I don't know whether complicated constructions using lists of lambdas will meet that goal.

comment:9 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:10 Changed 5 years ago by git

  • Commit changed from c7c7b0cbcc90ad04f872507418a3fb3c0ed2db73 to 38cce3dcd24ce9b8096fda496e0bc2babbf7b293

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

38cce3dremove cmp in binary_code, codecan, etc

comment:11 Changed 5 years ago by jdemeyer

  • 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 chapoton

Thanks.

For morphism, I understand and agree.

The other one was good I think.

comment:13 Changed 5 years ago by vbraun

  • Branch changed from public/21176 to 38cce3dcd24ce9b8096fda496e0bc2babbf7b293
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.