Opened 7 years ago
Closed 4 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 7 years ago by
- Branch set to u/aapitzsch/ticket/17175
- Commit set to 8e6a4890f80a9cbab620a777495dd7ac27d81d5d
- Status changed from new to needs_review
comment:2 follow-up: ↓ 6 Changed 7 years ago by
- 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.*
, 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 7 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 7 years ago by
- Commit changed from 8e6a4890f80a9cbab620a777495dd7ac27d81d5d to caf728dcd3eb43b67d9da6332992a3139a62b050
comment:5 Changed 7 years ago by
- Commit changed from caf728dcd3eb43b67d9da6332992a3139a62b050 to d419515eab7cade4ef3983159d87dd3b10998544
comment:6 in reply to: ↑ 2 Changed 7 years ago by
- 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.*
, 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 7 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 in reply to: ↑ 7 Changed 7 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 7 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 6 years ago by
- Status changed from needs_review to needs_work
- Work issues set to merge conflicts
comment:11 Changed 6 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 6 years ago by
- 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 5 years ago by
- Component changed from misc to python3
comment:14 Changed 4 years ago by
- Milestone changed from sage-6.4 to sage-8.0
comment:15 Changed 4 years ago by
- Milestone changed from sage-8.0 to sage-duplicate/invalid/wontfix
comment:16 Changed 4 years ago by
- 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 4 years ago by
- Resolution set to wontfix
- Status changed from positive_review to closed
New commits:
replace __cmp__ by rich comparisons