Replace some __cmp__() by rich comparison

[a-andre]

This is part of #16537.

Component: python3

Work Issues: merge conflicts

Author: André Apitzsch

Branch/Commit: u/aapitzsch/ticket/17175 @ d419515

Reviewer: Frédéric Chapoton

Issue created by migration from https://trac.sagemath.org/ticket/17175

[a-andre]

New commits:

8e6a489

replace `__cmp__` by rich comparisons

[a-andre]

Branch: u/aapitzsch/ticket/17175

[a-andre]

Commit: 8e6a489

[ohanar]

Reviewer: R. Andrew Ohana

[ohanar]

comment:2

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).

[a-andre]

comment:3

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.

[sagetrac-git]

Changed commit from 8e6a489 to caf728d

[sagetrac-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__

[sagetrac-git]

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

1d1dabf	fix last commit
d419515	add missing comparisons

[sagetrac-git]

Changed commit from caf728d to d419515

[a-andre]

comment:6

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]

comment:7

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?

[a-andre]

comment:8

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/[email protected]/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]

comment:9

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!

[mezzarobba]

Work Issues: merge conflicts

[rwst]

comment:11

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.

[rwst]

comment:12

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

[fchapoton]

comment:16

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

[fchapoton]

Changed reviewer from R. Andrew Ohana to Frédéric Chapoton