Opened 8 years ago
Closed 5 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, GitHub, GitLab) | Commit: | d419515eab7cade4ef3983159d87dd3b10998544 |
Dependencies: | Stopgaps: |
Description
This is part of #16537.
Change History (17)
comment:1 Changed 8 years ago by
Branch: | → u/aapitzsch/ticket/17175 |
---|---|
Commit: | → 8e6a4890f80a9cbab620a777495dd7ac27d81d5d |
Status: | new → needs_review |
comment:2 follow-up: 6 Changed 8 years ago by
Reviewers: | → R. Andrew Ohana |
---|---|
Status: | needs_review → 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.*
, andsage.rings.finit_rings.residue_field
, usingnot isinstance(other, type(self))
is not equivalent to the previous line of reasoning (i.e.type(self)
might be a super class oftype(other)
). Instead use something likeother.__class__ is not self.__class__
(using__class__
is better thantype
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 8 years ago by
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 8 years ago by
Commit: | 8e6a4890f80a9cbab620a777495dd7ac27d81d5d → caf728dcd3eb43b67d9da6332992a3139a62b050 |
---|
comment:5 Changed 8 years ago by
Commit: | caf728dcd3eb43b67d9da6332992a3139a62b050 → d419515eab7cade4ef3983159d87dd3b10998544 |
---|
comment:6 Changed 8 years ago by
Status: | needs_work → 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.*
, andsage.rings.finit_rings.residue_field
, usingnot isinstance(other, type(self))
is not equivalent to the previous line of reasoning (i.e.type(self)
might be a super class oftype(other)
). Instead use something likeother.__class__ is not self.__class__
(using__class__
is better thantype
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: 8 Changed 8 years ago by
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 Changed 8 years ago by
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 decoratorfunctools.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 8 years ago by
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 8 years ago by
Status: | needs_review → needs_work |
---|---|
Work issues: | → merge conflicts |
comment:11 Changed 8 years ago by
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 8 years ago by
Status: | needs_work → needs_info |
---|
With the new arguments in #16537 I think this is at least needs_info, if not wontfix.
comment:13 Changed 7 years ago by
Component: | misc → python3 |
---|
comment:14 Changed 6 years ago by
Milestone: | sage-6.4 → sage-8.0 |
---|
comment:15 Changed 6 years ago by
Milestone: | sage-8.0 → sage-duplicate/invalid/wontfix |
---|
comment:16 Changed 5 years ago by
Reviewers: | R. Andrew Ohana → Frédéric Chapoton |
---|---|
Status: | needs_info → positive_review |
This is way outdated. Let it be closed as invalid.
comment:17 Changed 5 years ago by
Resolution: | → wontfix |
---|---|
Status: | positive_review → closed |
New commits:
replace __cmp__ by rich comparisons