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)

classic-division.txt (92.5 KB) - added by wluebbe 7 years ago.
Depreciation warnings for 'classic int division' (from "-3")

Download all attachments as: .zip

Change History (14)

comment:1 Changed 7 years ago by aapitzsch

  • Branch set to u/aapitzsch/ticket/16371
  • Commit set to 83324d7298548555ef6547bca4e7501c29365fdc
  • Status changed from new to needs_review

comment:2 follow-up: Changed 7 years ago by 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 also tried to estimate how complete your patch might be. I ran futurize -w -f division src/sage that inserted form __future__ import division into 401 py module. I compared the test results from your patch vs. the current develop branch:

Aspect develop / 16371develop + division 16471 + division
runtime [sec] 2,573 7,870 3,435
failures 0 8,776 2,820
log lines 5,090 24,198,439 72,581

So this this is already quite a progress :-)

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

Changed 7 years ago by wluebbe

Depreciation warnings for 'classic int division' (from "-3")

comment:3 in reply to: ↑ 2 Changed 7 years ago by aapitzsch

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 wluebbe

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 / 16371develop + 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 aapitzsch

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 git

  • Commit changed from 83324d7298548555ef6547bca4e7501c29365fdc to b73d411819a29795e7efe2e25ff88cb903d61919

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

b73d411trac #16371: reviewer patch - some PEP8 readability improvements

comment:7 Changed 7 years ago by wluebbe

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

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

  • Status changed from positive_review to needs_work

Merge conflict, please merge in the next beta

comment:10 Changed 7 years ago by git

  • Commit changed from b73d411819a29795e7efe2e25ff88cb903d61919 to f848a660ea84e899515e8e9532a1b64fb4550ff9

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

f848a66Merge remote-tracking branch 'origin/develop' into py3_division

comment:11 Changed 7 years ago by aapitzsch

  • Status changed from needs_work to needs_review

Merged.

comment:12 Changed 7 years ago by wluebbe

  • Status changed from needs_review to positive_review

Checked and tested again after merge.

comment:13 Changed 7 years ago by vbraun

  • Branch changed from u/aapitzsch/ticket/16371 to f848a660ea84e899515e8e9532a1b64fb4550ff9
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.