Replace some __cmp__() by rich comparison

[aapitzsch]

This is part of #16537.

[aapitzsch]

New commits:

​8e6a489

replace __cmp__ by rich comparisons

[ohanar]

Thanks for working on this. I haven't looked through everything thoroughly yet, but here are some initial comments/questions:

• In sage.combinat.e_one_star (and in general), why do you change the expression in __eq__ method to use backslashes instead of parens? According to PEP8, wrapping long lines should be done with parens, brackets, and braces rather than with backslashes.

• In sage.combinat.root_system.branching_rules, sage.doctest.*, and sage.rings.finit_rings.residue_field, using not isinstance(other, type(self)) is not equivalent to the previous line of reasoning (i.e. type(self) might be a super class of type(other)). Instead use something like other.__class__ is not self.__class__ (using __class__ is better than type for duck typing purposes).

• In sage.combinat.root_system.type_irreducible, it seems to me that the added __ne__ method is unecessary.

• In sage.dynamics.flat_surfaces.strata, you seemed to have removed the less than and greater than functionality (that wasn't being tested by the doctests).

[aapitzsch]

Removing __ne__ from sage.combinat.root_system.type_reducible raises doctest errors like

sage -t src/sage/combinat/root_system/branching_rules.py
**********************************************************************
File "src/sage/combinat/root_system/branching_rules.py", line 667, in sage.combinat.root_system.branching_rules.branch_weyl_character
Failed example:
    F4(0,0,1,0).branch(G2xA1,rule="miscellaneous")
Exception raised:
    Traceback (most recent call last):
      File "/opt/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 488, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/opt/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 851, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.combinat.root_system.branching_rules.branch_weyl_character[130]>", line 1, in <module>
        F4(Integer(0),Integer(0),Integer(1),Integer(0)).branch(G2xA1,rule="miscellaneous")
      File "/opt/sage/local/lib/python2.7/site-packages/sage/combinat/root_system/weyl_characters.py", line 1103, in branch
        return sage.combinat.root_system.branching_rules.branch_weyl_character(self, self.parent(), S, rule=rule)
      File "/opt/sage/local/lib/python2.7/site-packages/sage/combinat/root_system/branching_rules.py", line 985, in branch_weyl_character
        raise ValueError("rule has wrong target Cartan type")
    ValueError: rule has wrong target Cartan type

We should keep it for now and check again when all __cmp__() have been removed.

[git]

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

​2b65bc8	use parentheses to wrap lines
​caf728d	use other.__class__ is not self.__class__

[git]

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

​1d1dabf	fix last commit
​d419515	add missing comparisons

[aapitzsch]

Replying to ohanar:

• In sage.combinat.e_one_star (and in general), why do you change the expression in __eq__ method to use backslashes instead of parens? According to PEP8, wrapping long lines should be done with parens, brackets, and braces rather than with backslashes.

Didn't know about it. Fixed now.

• In sage.combinat.root_system.branching_rules, sage.doctest.*, and sage.rings.finit_rings.residue_field, using not isinstance(other, type(self)) is not equivalent to the previous line of reasoning (i.e. type(self) might be a super class of type(other)). Instead use something like other.__class__ is not self.__class__ (using __class__ is better than type for duck typing purposes).

Fixed this.

• In sage.dynamics.flat_surfaces.strata, you seemed to have removed the less than and greater than functionality (that wasn't being tested by the doctests).

Added functions and tests.

[wluebbe]

Hi André, how did you select the modules from the mountain of affected modules from ticket:16537? Is there a common theme? Just curious!

By the way, after seeing the size of the cmp-problem in ticket:16537 I was so shocked that I stopped working on Sage and Python 3! So this gives me some fresh motivation ...

What is the reason for explicitly defining all 4 comparisons in src/sage/dynamics/flat_surfaces/strata.py instead of defining one (e.g. __gt__) and using the class decorator functools.total_ordering?

[aapitzsch]

Replying to wluebbe:

Hi André, how did you select the modules from the mountain of affected modules from ticket:16537? Is there a common theme? Just curious!

Up to some exceptions I chose the __cmp__() functions which only checked for equality, where I hadn't to implement __gt__() etc.

By the way, after seeing the size of the cmp-problem in ticket:16537 I was so shocked that I stopped working on Sage and Python 3! So this gives me some fresh motivation ...

The main "problem" of replacing __cmp__() is pointed out in ​http://www.mail-archive.com/sage-devel@googlegroups.com/msg73051.html. Until we find a reasonable solution for it, we should continue with the easier tickets, like #16529.

What is the reason for explicitly defining all 4 comparisons in src/sage/dynamics/flat_surfaces/strata.py instead of defining one (e.g. __gt__) and using the class decorator functools.total_ordering?

All comparisons need self._marked_separatrix == other._marked_separatrix and I think this leads to problems with the class decorator.

[wluebbe]

I do not think that it is always the right approach to solve the easier problems before the hard ones.

We have done this with good reason for stage 1 (#15980). But the completion of stage 1 has to precede the completion of stage 2. And now we have the hard problems that remain in stage 1!

I think that stage 2 (#16052) could use a tool like ​https://github.com/PythonCharmers/python-future to good effect. Clearly this does not prevent us from solving some of the problems (added originally to stage 2) in a stage 1 style (as was possible for filter and reduce, and I think it is also possible for map #16073!).

But we should try to complete stage 1 as soon as possible: if we cannot solve those remaining hard problems then Sage will not available under Python 3.

Anyway I will have a look at #16529 ;-) And I will try to grok the rich comparison stuff - it is difficult and but it is also necessary!

[rws]

Same question as in #16537: Can the work be reduced by including the code in ​http://www.voidspace.org.uk/python/articles/comparison.shtml into the SageObject class? Please reply there.

[rws]

With the new arguments in #16537 I think this is at least needs_info, if not wontfix.

[chapoton]

This is way outdated. Let it be closed as invalid.