Opened 7 years ago
Closed 7 years ago
#16371 closed enhancement (fixed)
Change some integer divisions from / to //
Reported by:  aapitzsch  Owned by:  

Priority:  major  Milestone:  sage6.3 
Component:  misc  Keywords:  python3 
Cc:  Merged in:  
Authors:  André Apitzsch  Reviewers:  Wilfried Luebbe 
Report Upstream:  N/A  Work issues:  
Branch:  f848a66 (Commits)  Commit:  f848a660ea84e899515e8e9532a1b64fb4550ff9 
Dependencies:  Stopgaps: 
Description
The attached patch will simplify the transition to a python3 compatible division semantic (see #15995)
Attachments (1)
Change History (14)
comment:1 Changed 7 years ago by
 Branch set to u/aapitzsch/ticket/16371
 Commit set to 83324d7298548555ef6547bca4e7501c29365fdc
 Status changed from new to needs_review
comment:2 followup: ↓ 3 Changed 7 years ago by
comment:3 in reply to: ↑ 2 Changed 7 years ago by
Replying to wluebbe:
Hi André,
I did a
./sage tp 4 all long >logs/sagetp4alllong16371
and got no failures :). Now I will look at all the 63 changed modules (since I am concerned that Sage has not enough (doc)tests).I had run the doctest with the "3" command line option (with mixed success). I attach part of the results as they are related to division. There is a pretty large overlap with your changes, but there is not a complete match!
Therefor I am curious: How did you find / determine the
/
that you changed into//
?
I went through the results of
. src/binsageenv python2 Qwarn ./local/bin/sageruntests long src/sage/SUBDIR
and tried to reduce the number of warnings.
comment:4 Changed 7 years ago by
Hi,
I've reviewed the 63 modules and it looked OK :)
Then I tried to find MORE division to change.
That resulted in patch u/wluebbe/ticket/16371
. The patch tested with no failures. But then I inserted the from __future__ import division
and run the tests again. I was hoping for an improvement, but the result is actually worse :(( (see the last column in the table compared with the previous column).
Aspect  develop / 16371  develop + division  16471 + division  develop + MORE + division 
runtime [sec]  2,573  7,870  3,435  3,603 
failures  0  8,776  2,820  3,065 
log lines  5,090  24,198,439  72,581  74,464 
So I did not put my branch into this ticket!
Can you see why the branch u/wluebbe/ticket/16371
did not result in visible progress? Or is the measurement misleading?
How to make progress and when do we know when we are done??
comment:5 Changed 7 years ago by
Have you changed __div__
to __truediv__
after inserting from __future__ import division
?
We probably have to update or patch some 3rd party libs which are still using the old __div__
. Otherwise calling their division functions will fail.
Let's fix the remaining divisions in #15995 or another ticket.
comment:6 Changed 7 years ago by
 Commit changed from 83324d7298548555ef6547bca4e7501c29365fdc to b73d411819a29795e7efe2e25ff88cb903d61919
Branch pushed to git repo; I updated commit sha1. New commits:
b73d411  trac #16371: reviewer patch  some PEP8 readability improvements

comment:7 Changed 7 years ago by
 Reviewers set to Wilfried Luebbe
No, I have changed nothing to __truediv__
. And that should be done only very carefully and with a clearly documented strategy.
I agree that the next step(s) should go into a separate ticket.
I will check (and test) this complete patch once more. Stay tuned.
comment:8 Changed 7 years ago by
 Status changed from needs_review to positive_review
All test pass and patch looks OK. It's a positive review for me.
comment:9 Changed 7 years ago by
 Status changed from positive_review to needs_work
Merge conflict, please merge in the next beta
comment:10 Changed 7 years ago by
 Commit changed from b73d411819a29795e7efe2e25ff88cb903d61919 to f848a660ea84e899515e8e9532a1b64fb4550ff9
Branch pushed to git repo; I updated commit sha1. New commits:
f848a66  Merge remotetracking branch 'origin/develop' into py3_division

comment:12 Changed 7 years ago by
 Status changed from needs_review to positive_review
Checked and tested again after merge.
comment:13 Changed 7 years ago by
 Branch changed from u/aapitzsch/ticket/16371 to f848a660ea84e899515e8e9532a1b64fb4550ff9
 Resolution set to fixed
 Status changed from positive_review to closed
Hi André,
I did a
./sage tp 4 all long >logs/sagetp4alllong16371
and got no failures :). Now I will look at all the 63 changed modules (since I am concerned that Sage has not enough (doc)tests).I had run the doctest with the "3" command line option (with mixed success). I attach part of the results as they are related to division. There is a pretty large overlap with your changes, but there is not a complete match!
Therefor I am curious: How did you find / determine the
/
that you changed into//
?I also tried to estimate how complete your patch might be. I ran
futurize w f division src/sage
that insertedform __future__ import division
into 401 py module. I compared the test results from your patch vs. the current develop branch:So this this is already quite a progress :)