Opened 5 years ago

Closed 2 years ago

#17175 closed enhancement (wontfix)

Replace some __cmp__() by rich comparison

Reported by: aapitzsch Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: python3 Keywords: python3
Cc: Merged in:
Authors: André Apitzsch Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues: merge conflicts
Branch: u/aapitzsch/ticket/17175 (Commits) Commit: d419515eab7cade4ef3983159d87dd3b10998544
Dependencies: Stopgaps:

Description

This is part of #16537.

Change History (17)

comment:1 Changed 5 years ago by aapitzsch

  • Branch set to u/aapitzsch/ticket/17175
  • Commit set to 8e6a4890f80a9cbab620a777495dd7ac27d81d5d
  • Status changed from new to needs_review

New commits:

8e6a489replace __cmp__ by rich comparisons

comment:2 follow-up: Changed 5 years ago by ohanar

  • Reviewers set to R. Andrew Ohana
  • Status changed from needs_review to needs_work

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

comment:3 Changed 5 years ago by 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.

comment:4 Changed 5 years ago by git

  • Commit changed from 8e6a4890f80a9cbab620a777495dd7ac27d81d5d to caf728dcd3eb43b67d9da6332992a3139a62b050

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

2b65bc8use parentheses to wrap lines
caf728duse other.__class__ is not self.__class__

comment:5 Changed 5 years ago by git

  • Commit changed from caf728dcd3eb43b67d9da6332992a3139a62b050 to d419515eab7cade4ef3983159d87dd3b10998544

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

1d1dabffix last commit
d419515add missing comparisons

comment:6 in reply to: ↑ 2 Changed 5 years ago by aapitzsch

  • Status changed from needs_work to needs_review

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.

comment:7 follow-up: Changed 5 years ago by wluebbe

  • Keywords python3 added

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?

comment:8 in reply to: ↑ 7 Changed 5 years ago by 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.

comment:9 Changed 5 years ago by 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!

comment:10 Changed 5 years ago by mmezzarobba

  • Status changed from needs_review to needs_work
  • Work issues set to merge conflicts

comment:11 Changed 5 years ago by 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.

comment:12 Changed 5 years ago by rws

  • Status changed from needs_work to needs_info

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

comment:13 Changed 4 years ago by jdemeyer

  • Component changed from misc to python3

comment:14 Changed 3 years ago by chapoton

  • Milestone changed from sage-6.4 to sage-8.0

comment:15 Changed 2 years ago by chapoton

  • Milestone changed from sage-8.0 to sage-duplicate/invalid/wontfix

comment:16 Changed 2 years ago by chapoton

  • Reviewers changed from R. Andrew Ohana to Frédéric Chapoton
  • Status changed from needs_info to positive_review

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

comment:17 Changed 2 years ago by embray

  • Resolution set to wontfix
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.