Opened 7 years ago
Closed 7 years ago
#18796 closed enhancement (fixed)
Python 3 preparation: Cleaning up the bit rot that occurred to a number of Python 3 fixes
Reported by:  wluebbe  Owned by:  

Priority:  major  Milestone:  sage6.8 
Component:  misc  Keywords:  python3 
Cc:  Merged in:  
Authors:  Wilfried Luebbe  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  780a382 (Commits, GitHub, GitLab)  Commit:  780a3828a1dd3f5557035ef8ed7a3efc28b77e68 
Dependencies:  Stopgaps: 
Description
A number of the Python 3 tickets that are tracked in #15980 and that had been closed have to be addressed again: instances of the migration problems that they fixed have surfaced again.
Change History (18)
comment:1 Changed 7 years ago by
 Branch set to u/wluebbe/18796
 Commit set to 4b62172372e9fb586ebc8637a971baff074d1d61
comment:2 Changed 7 years ago by
 Commit changed from 4b62172372e9fb586ebc8637a971baff074d1d61 to 3b23e78b2387240a738862417fe66070856c2c68
Branch pushed to git repo; I updated commit sha1. New commits:
e23c1bc  Trac #18796: Follow up trac #15986  Change names of some method attributes

4743f30  Trac #18796: Follow up trac #15743  <> is not Python 3 compatible

28e4faa  Trac #18796: Follow up trac #15990  Change syntax of raise statement

b70bb8d  Trac #18796: Follow up trac #16078  reduce() is no more a builtin function

3b23e78  Trac #18796: Follow up trac #15993  Remove implicit tuple parameter unpacking

comment:3 Changed 7 years ago by
 Status changed from new to needs_review
The next 5 commits follow up these tickets
#15986  Change names of some method attributes 2to3 w f methodattrs src/sage
#15743  <> is not Python 3 compatible 2to3 w f ne src/sage
#15990  Change syntax of raise statement 2to3 w f raise src/sage
#16078  reduce() is no more a builtin function 2to3 w f reduce src/sage
#15993  Remove implicit tuple parameter unpacking 2to3 w f tuple_params src/sage
Three proposed module changes by raise
involve raise with_traceback()
:
src/sage/interfaces/expect.py
src/sage/interfaces/singular.py
src/sage/sage/schemes/elliptic_curves/ell_number_field.py
This requires the use of a compatibility library and therefor belongs to stage 2 (see metaticket #16052). This 3 modules will be handled in a separate ticket #18799.
comment:4 Changed 7 years ago by
 Summary changed from Python 3 preparation: Cleaning up the bit rot that occurred to a number of Python fixes to Python 3 preparation: Cleaning up the bit rot that occurred to a number of Python 3 fixes
comment:5 followup: ↓ 7 Changed 7 years ago by
 Status changed from needs_review to needs_work
Please fix the formatting of
if ( isinstance(self, type(other))\ and self._ambient_space_functor == other._ambient_space_functor\ and self._generators == other._generators ):
and others in src/sage/modular/modform_hecketriangle/functors.py
.
\
is not necessary.
comment:6 followup: ↓ 8 Changed 7 years ago by
Why do you replace
type(self) == type(other)
by
isinstance(self, type(other))
That doesn't seem correct (the 2 are certainly not equivalent)
comment:7 in reply to: ↑ 5 Changed 7 years ago by
Replying to aapitzsch:
Please fix the formatting of
...
and others in
src/sage/modular/modform_hecketriangle/functors.py
.\
is not necessary.
The change was generated by 2to3 w f idioms src/sage
. I did not change any formatting here as in some previous Python3 ticket I was asked by the reviewer to undo those improvements as they were not strictly required by the task at hand. I complied :/
I agree that the mentioned code is not pretty and not quite PEP8 compliant. But before fixing it I'd like to discuss comment:6.
comment:8 in reply to: ↑ 6 ; followup: ↓ 9 Changed 7 years ago by
Replying to jdemeyer:
Why do you replace ...
All the changes were generated by 2to3 w f idioms src/sage
.
While type(self) == type(other)
means strict type identity the expression isinstance(self, type(other))
allows subclasses. The Python documentation (https://docs.python.org/2.7/library/functions.html?highlight=type#type) says: The isinstance() builtin function is recommended for testing the type of an object.
There was one generated change (see comment:1) that I reverted because of failing doctests. Failing of doctests was the only method I used to determine that the change was not correct. I have no deep understanding of the code.
Therefor one can reason that without more insight all changes from type()
to isinstance()
should be reverted.
Or to keep them until proved guilty.
Remark: In ticket #15984 we made a large number of such changes supported by the same reasoning. This was in Sage 6.2 15 month ago. So there is some evidence that the approach is reasonable.
comment:9 in reply to: ↑ 8 Changed 7 years ago by
Replying to wluebbe:
I have no deep understanding of the code.
You should really never make changes that you don't understand.
Therefor one can reason that without more insight all changes from
type()
toisinstance()
should be reverted.
Indeed.
comment:10 Changed 7 years ago by
 Commit changed from 3b23e78b2387240a738862417fe66070856c2c68 to 686e31234c17629d087c61a40e643a4de14e816f
Branch pushed to git repo; I updated commit sha1. New commits:
686e312  Trac #18796: Undo the changes from type() to isinstance()

comment:11 Changed 7 years ago by
 Status changed from needs_work to needs_review
This commit undoes the changes from type()
to isinstance()
in the 6 affected modules.
I recommend not to employ 2to3 w f idioms src/sage
anymore since those changes not essential to Py3 compatibility and not always correct. But they improve the code if used with sufficient care.
comment:12 Changed 7 years ago by
 Status changed from needs_review to needs_work
(x_y[0],x_y[1])
is just x_y
comment:13 Changed 7 years ago by
Don't remove comments like # make sure that the first two columns are "11, 12, ..., 1n, 21, 22, ..."
comment:14 Changed 7 years ago by
If you're changing func_code
to __code__
, you also need to change
hasattr(obj, 'func_code')
to
hasattr(obj, '__code__')
comment:15 Changed 7 years ago by
 Commit changed from 686e31234c17629d087c61a40e643a4de14e816f to 780a3828a1dd3f5557035ef8ed7a3efc28b77e68
Branch pushed to git repo; I updated commit sha1. New commits:
780a382  Trac #18796: Addressed reviewer comments and some more polishing

comment:16 Changed 7 years ago by
 Status changed from needs_work to needs_review
 Changed
(x_y[0],x_y[1])
inbidb.py
 Added removed comment in
latin_squares.py
 Changed
func_code
insageinspect.py
 Changed
has_key
inintegrable_representations.py
 Changed type equality tests into type identity tests in
functors.py
,graded_ring_element.py
anddisplay_manager.py
. This similar to #18809.  Stripped lots of trailing whitespace in
display_manager.py
comment:17 Changed 7 years ago by
 Reviewers set to Jeroen Demeyer
 Status changed from needs_review to positive_review
comment:18 Changed 7 years ago by
 Branch changed from u/wluebbe/18796 to 780a3828a1dd3f5557035ef8ed7a3efc28b77e68
 Resolution set to fixed
 Status changed from positive_review to closed
These first 5 commits follow up the tickets (again using
2to3
: #15982 Change the syntax of the except clause2to3 w f except src/sage
#16067 Semantic of filter() function changed2to3 w f filter src/sage
#15983 Sames of some function attributes2to3 w f funcattrs src/sage
#15784 Usein
instead ofhas_key()
2to3 w f has_key src/sage
#15984 Use more modern Python idioms2to3 w f idioms src/sage
I had to backout the proposed change for
src/sage/game_theory/normal_form_game.py
(see also the 4 reverted modules mentioned in #15984).New commits:
Trac #18796: Followup on trac #15982 Change the syntax of the except clause
Trac #18796: Followup on trac #16067  semantic of filter() function changed
Trac #18796: Followup on trac #15983  names of some function attributes
Trac #18796: Followup on trac #15784  Use `in` instead of `has_key()`
Trac #18796: Followup on trac #15984  Use more modern Python idioms