Opened 8 years ago

Closed 7 years ago

Last modified 5 years ago

#18659 closed enhancement (fixed)

Python 3 preparation: Change more integer divisions from / to // (part 2)

Reported by: wluebbe Owned by:
Priority: major Milestone: sage-7.2
Component: python3 Keywords: divison
Cc: aapitzsch, tscrim, jdemeyer Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: ee97a51 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

GitHub link to the corresponding issue

Description (last modified by chapoton)

The attached patch is part of the enabling of Sage for Py2/3. Py2 integer division / is changed to P2/3 floor division // (see #15995). This ticket is similar to #16371 which already performed such changes.

see also #20471, #20483, #20468

Change History (28)

comment:1 Changed 7 years ago by chapoton

Authors: Frédéric Chapoton
Branch: u/chapoton/18659
Cc: tscrim jdemeyer added
Commit: 4c5457d5de7bf50dd0eb863037d7e2bf40020e6c
Description: modified (diff)
Status: newneeds_review

New commits:

4c5457dhandling some more future divisions

comment:2 Changed 7 years ago by git

Commit: 4c5457d5de7bf50dd0eb863037d7e2bf40020e6cf9f15bb2ae30ab0b7eda2a3e8b6a5cdbcf4ff18b

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

f9f15bbsome details

comment:3 Changed 7 years ago by chapoton

Milestone: sage-6.8sage-7.2

comment:4 Changed 7 years ago by git

Commit: f9f15bb2ae30ab0b7eda2a3e8b6a5cdbcf4ff18b3527969a971c482cbdbd5d37d40862d58b5bc8c6

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

3527969wundo wrong change in crystals

comment:5 Changed 7 years ago by git

Commit: 3527969a971c482cbdbd5d37d40862d58b5bc8c6aaf1a1226623645957cfada9e4b45d3c29b34706

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

aaf1a12another try for division in kirillov reshetikhin crystals

comment:6 Changed 7 years ago by jdemeyer

Status: needs_reviewneeds_work

The line

them does euclidean division (in Python2) or float division (in Python3)::

does not correspond to the code which does //.

comment:7 Changed 7 years ago by jdemeyer

``2//3`` return ``0``.: return should be returns

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

comment:8 Changed 7 years ago by jdemeyer

I feel that this ticket does too much at once. Most changes are non-trivial and require some work to review. It's too much for me anyway.

comment:9 Changed 7 years ago by chapoton

should I split it in smaller chunks ? it is already only a small step in all the changes that have to be made...

comment:10 Changed 7 years ago by chapoton

Component: miscpython3

comment:11 Changed 7 years ago by git

Commit: aaf1a1226623645957cfada9e4b45d3c29b34706b5ab2234ed86b6792b6e83f03b520c94b2d0d851

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

b5ab223trac 18659 better corrections in rst files

comment:12 Changed 7 years ago by chapoton

Status: needs_workneeds_review

comment:13 Changed 7 years ago by tscrim

I don't think your changes in rigged_configurations/* are correct as they should all be Sage integers and, in certain cases, allowed to be rationals. Although perhaps it ends up being okay as there are no doctest failures, but I need to check (which I am doing now).

comment:14 Changed 7 years ago by tscrim

Status: needs_reviewneeds_work

Actually, it does seem to handle the division to rationals okay, but now it has values which are floats that should be integers.

comment:15 Changed 7 years ago by tscrim

Branch: u/chapoton/18659u/tscrim/18659
Commit: b5ab2234ed86b6792b6e83f03b520c94b2d0d8518de2be429f99c72ca032480b4207b888f4052297
Status: needs_workneeds_review

Okay, this should take care of the issues in rigged_configurations/*.


New commits:

8de2be4Fixing division in rigged configurations.

comment:16 in reply to:  9 Changed 7 years ago by jdemeyer

Replying to chapoton:

should I split it in smaller chunks ? it is already only a small step in all the changes that have to be made...

I cannot speak for others, but for me it would improve the chances that I will review it.

comment:17 Changed 7 years ago by jdemeyer

Splitting the ticket also means that we don't need an "all or nothing" approach. For example #15994 was rejected because of problems in one particular module. That is a pity, because the other changes are probably good.

comment:18 Changed 7 years ago by chapoton

Branch: u/tscrim/18659public/18659
Commit: 8de2be429f99c72ca032480b4207b888f4052297ee97a5182ac711dc20ada1942c38f6ecd8a5013a

I have split into 2 tickets, the other one being #20533 for the combinat folder


New commits:

11629f1Merge branch 'u/tscrim/18659' into 7.2.b6
ee97a51splitting trac 18659 in smaller chunks

comment:19 Changed 7 years ago by chapoton

I think only the changes in

*1 algebras/steenrod/steenrod_algebra_bases.py *2 graphs/generic_graph.py *3 modular/arithgroup/arithgroup_perm.py

really need to be checked seriously. All others are very simple.

For *1, there is a sentence that explain that all dimensions are divided by the given factor. Being dimensions, they surely must be integers.

For *2, the end result must be an integer (Wiener index), and we are simply dividing an even integer by 2.

For *3, it seems to be about cusp widths, that are in principle integers, as far as I understand.

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

comment:20 Changed 7 years ago by tscrim

Reviewers: Travis Scrimshaw
Status: needs_reviewpositive_review

As far as I can tell, these are correct.

comment:21 Changed 7 years ago by chapoton

Thanks a lot.

comment:22 Changed 7 years ago by vbraun

Branch: public/18659ee97a5182ac711dc20ada1942c38f6ecd8a5013a
Resolution: fixed
Status: positive_reviewclosed

comment:23 Changed 7 years ago by chapoton

Commit: ee97a5182ac711dc20ada1942c38f6ecd8a5013a

This ticket has had seriously wrong consequences about the work done in #20533.

comment:24 Changed 7 years ago by chapoton

This ticket's changes are plain wrong !

EDIT: can we easily undo this, quickly, please ??

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

comment:25 Changed 7 years ago by vbraun

This is already in 7.3.beta0 so we can't undo it. You added 488db3dddfa94779c0ba612fd552239aec80ea5d that reverts some changes, so thats what you got.

comment:26 Changed 7 years ago by chapoton

Yes, I guess that's my fault. Jeroen asked me to split a ticket, so I did it (as I could) by reverting some changes, and putting them in another ticket. Then the other ticket went in sage first, and this one after. And just undid everything done in the other ticket.

I will have to re-do the division clean-up in combinat. I am not happy about that.

comment:27 Changed 7 years ago by vbraun

you can use git revert

comment:28 Changed 5 years ago by embray

Keywords: divison added; python3 removed
Note: See TracTickets for help on using tickets.