#18659 closed enhancement (fixed)
Python 3 preparation: Change more integer divisions from / to // (part 2)
Reported by:  wluebbe  Owned by:  

Priority:  major  Milestone:  sage7.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)  Commit:  
Dependencies:  Stopgaps: 
Change History (28)
comment:1 Changed 5 years ago by
 Branch set to u/chapoton/18659
 Cc tscrim jdemeyer added
 Commit set to 4c5457d5de7bf50dd0eb863037d7e2bf40020e6c
 Description modified (diff)
 Status changed from new to needs_review
comment:2 Changed 5 years ago by
 Commit changed from 4c5457d5de7bf50dd0eb863037d7e2bf40020e6c to f9f15bb2ae30ab0b7eda2a3e8b6a5cdbcf4ff18b
Branch pushed to git repo; I updated commit sha1. New commits:
f9f15bb  some details

comment:3 Changed 5 years ago by
 Milestone changed from sage6.8 to sage7.2
comment:4 Changed 5 years ago by
 Commit changed from f9f15bb2ae30ab0b7eda2a3e8b6a5cdbcf4ff18b to 3527969a971c482cbdbd5d37d40862d58b5bc8c6
Branch pushed to git repo; I updated commit sha1. New commits:
3527969  wundo wrong change in crystals

comment:5 Changed 5 years ago by
 Commit changed from 3527969a971c482cbdbd5d37d40862d58b5bc8c6 to aaf1a1226623645957cfada9e4b45d3c29b34706
Branch pushed to git repo; I updated commit sha1. New commits:
aaf1a12  another try for division in kirillov reshetikhin crystals

comment:6 Changed 5 years ago by
 Status changed from needs_review to needs_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 5 years ago by
``2//3`` return ``0``.
: return
should be returns
comment:8 Changed 5 years ago by
I feel that this ticket does too much at once. Most changes are nontrivial and require some work to review. It's too much for me anyway.
comment:9 followup: ↓ 16 Changed 5 years ago by
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 5 years ago by
 Component changed from misc to python3
comment:11 Changed 5 years ago by
 Commit changed from aaf1a1226623645957cfada9e4b45d3c29b34706 to b5ab2234ed86b6792b6e83f03b520c94b2d0d851
Branch pushed to git repo; I updated commit sha1. New commits:
b5ab223  trac 18659 better corrections in rst files

comment:12 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:13 Changed 5 years ago by
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 5 years ago by
 Status changed from needs_review to needs_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 5 years ago by
 Branch changed from u/chapoton/18659 to u/tscrim/18659
 Commit changed from b5ab2234ed86b6792b6e83f03b520c94b2d0d851 to 8de2be429f99c72ca032480b4207b888f4052297
 Status changed from needs_work to needs_review
Okay, this should take care of the issues in rigged_configurations/*
.
New commits:
8de2be4  Fixing division in rigged configurations.

comment:16 in reply to: ↑ 9 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
 Branch changed from u/tscrim/18659 to public/18659
 Commit changed from 8de2be429f99c72ca032480b4207b888f4052297 to ee97a5182ac711dc20ada1942c38f6ecd8a5013a
comment:19 Changed 5 years ago by
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.
comment:20 Changed 5 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
As far as I can tell, these are correct.
comment:21 Changed 5 years ago by
Thanks a lot.
comment:22 Changed 5 years ago by
 Branch changed from public/18659 to ee97a5182ac711dc20ada1942c38f6ecd8a5013a
 Resolution set to fixed
 Status changed from positive_review to closed
comment:23 Changed 5 years ago by
 Commit ee97a5182ac711dc20ada1942c38f6ecd8a5013a deleted
This ticket has had seriously wrong consequences about the work done in #20533.
comment:24 Changed 5 years ago by
This ticket's changes are plain wrong !
EDIT: can we easily undo this, quickly, please ??
comment:25 Changed 5 years ago by
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 5 years ago by
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 redo the division cleanup in combinat. I am not happy about that.
comment:27 Changed 5 years ago by
you can use git revert
comment:28 Changed 3 years ago by
 Keywords divison added; python3 removed
New commits:
handling some more future divisions