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: | sage-6.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 follow-up: ↓ 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/sage-tp4-all-long-16371
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/binsage-env python2 -Qwarn ./local/bin/sage-runtests --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 remote-tracking 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/sage-tp4-all-long-16371
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 :-)