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: sage-6.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:

Status badges

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 wluebbe

  • Branch set to u/wluebbe/18796
  • Commit set to 4b62172372e9fb586ebc8637a971baff074d1d61

These first 5 commits follow up the tickets (again using 2to3: #15982 Change the syntax of the except clause 2to3 -w -f except src/sage #16067 Semantic of filter() function changed 2to3 -w -f filter src/sage #15983 Sames of some function attributes 2to3 -w -f funcattrs src/sage #15784 Use in instead of has_key() 2to3 -w -f has_key src/sage #15984 Use more modern Python idioms 2to3 -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:

7b67376Trac #18796: Followup on trac #15982 Change the syntax of the except clause
ce787edTrac #18796: Followup on trac #16067 - semantic of filter() function changed
70e0dccTrac #18796: Followup on trac #15983 - names of some function attributes
f75c09bTrac #18796: Followup on trac #15784 - Use `in` instead of `has_key()`
4b62172Trac #18796: Followup on trac #15984 - Use more modern Python idioms

comment:2 Changed 7 years ago by git

  • Commit changed from 4b62172372e9fb586ebc8637a971baff074d1d61 to 3b23e78b2387240a738862417fe66070856c2c68

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

e23c1bcTrac #18796: Follow up trac #15986 - Change names of some method attributes
4743f30Trac #18796: Follow up trac #15743 - <> is not Python 3 compatible
28e4faaTrac #18796: Follow up trac #15990 - Change syntax of raise statement
b70bb8dTrac #18796: Follow up trac #16078 - reduce() is no more a builtin function
3b23e78Trac #18796: Follow up trac #15993 - Remove implicit tuple parameter unpacking

comment:3 Changed 7 years ago by wluebbe

  • Authors set to Wilfried Luebbe
  • 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 meta-ticket #16052). This 3 modules will be handled in a separate ticket #18799.

Last edited 7 years ago by wluebbe (previous) (diff)

comment:4 Changed 7 years ago by wluebbe

  • 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 follow-up: Changed 7 years ago by aapitzsch

  • 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 follow-up: Changed 7 years ago by jdemeyer

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 wluebbe

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 ; follow-up: Changed 7 years ago by wluebbe

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() built-in 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 jdemeyer

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() to isinstance() should be reverted.

Indeed.

comment:10 Changed 7 years ago by git

  • Commit changed from 3b23e78b2387240a738862417fe66070856c2c68 to 686e31234c17629d087c61a40e643a4de14e816f

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

686e312Trac #18796: Undo the changes from type() to isinstance()

comment:11 Changed 7 years ago by wluebbe

  • 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 jdemeyer

  • 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 jdemeyer

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 jdemeyer

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 git

  • Commit changed from 686e31234c17629d087c61a40e643a4de14e816f to 780a3828a1dd3f5557035ef8ed7a3efc28b77e68

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

780a382Trac #18796: Addressed reviewer comments and some more polishing

comment:16 Changed 7 years ago by wluebbe

  • Status changed from needs_work to needs_review
  • Changed (x_y[0],x_y[1]) in bidb.py
  • Added removed comment in latin_squares.py
  • Changed func_code in sageinspect.py
  • Changed has_key in integrable_representations.py
  • Changed type equality tests into type identity tests in functors.py, graded_ring_element.py and display_manager.py. This similar to #18809.
  • Stripped lots of trailing whitespace in display_manager.py

comment:17 Changed 7 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:18 Changed 7 years ago by vbraun

  • Branch changed from u/wluebbe/18796 to 780a3828a1dd3f5557035ef8ed7a3efc28b77e68
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.