#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: |
Change History (28)
comment:1 Changed 7 years ago by
Authors: | → Frédéric Chapoton |
---|---|
Branch: | → u/chapoton/18659 |
Cc: | tscrim jdemeyer added |
Commit: | → 4c5457d5de7bf50dd0eb863037d7e2bf40020e6c |
Description: | modified (diff) |
Status: | new → needs_review |
comment:2 Changed 7 years ago by
Commit: | 4c5457d5de7bf50dd0eb863037d7e2bf40020e6c → f9f15bb2ae30ab0b7eda2a3e8b6a5cdbcf4ff18b |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
f9f15bb | some details
|
comment:3 Changed 7 years ago by
Milestone: | sage-6.8 → sage-7.2 |
---|
comment:4 Changed 7 years ago by
Commit: | f9f15bb2ae30ab0b7eda2a3e8b6a5cdbcf4ff18b → 3527969a971c482cbdbd5d37d40862d58b5bc8c6 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
3527969 | wundo wrong change in crystals
|
comment:5 Changed 7 years ago by
Commit: | 3527969a971c482cbdbd5d37d40862d58b5bc8c6 → aaf1a1226623645957cfada9e4b45d3c29b34706 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
aaf1a12 | another try for division in kirillov reshetikhin crystals
|
comment:6 Changed 7 years ago by
Status: | needs_review → needs_work |
---|
The line
them does euclidean division (in Python2) or float division (in Python3)::
does not correspond to the code which does //
.
comment:8 Changed 7 years ago by
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 follow-up: 16 Changed 7 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 7 years ago by
Component: | misc → python3 |
---|
comment:11 Changed 7 years ago by
Commit: | aaf1a1226623645957cfada9e4b45d3c29b34706 → b5ab2234ed86b6792b6e83f03b520c94b2d0d851 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
b5ab223 | trac 18659 better corrections in rst files
|
comment:12 Changed 7 years ago by
Status: | needs_work → needs_review |
---|
comment:13 Changed 7 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 7 years ago by
Status: | needs_review → 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 7 years ago by
Branch: | u/chapoton/18659 → u/tscrim/18659 |
---|---|
Commit: | b5ab2234ed86b6792b6e83f03b520c94b2d0d851 → 8de2be429f99c72ca032480b4207b888f4052297 |
Status: | needs_work → needs_review |
Okay, this should take care of the issues in rigged_configurations/*
.
New commits:
8de2be4 | Fixing division in rigged configurations.
|
comment:16 Changed 7 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 7 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 7 years ago by
Branch: | u/tscrim/18659 → public/18659 |
---|---|
Commit: | 8de2be429f99c72ca032480b4207b888f4052297 → ee97a5182ac711dc20ada1942c38f6ecd8a5013a |
comment:19 Changed 7 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 7 years ago by
Reviewers: | → Travis Scrimshaw |
---|---|
Status: | needs_review → positive_review |
As far as I can tell, these are correct.
comment:22 Changed 7 years ago by
Branch: | public/18659 → ee97a5182ac711dc20ada1942c38f6ecd8a5013a |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
comment:23 Changed 7 years ago by
Commit: | ee97a5182ac711dc20ada1942c38f6ecd8a5013a |
---|
This ticket has had seriously wrong consequences about the work done in #20533.
comment:24 Changed 7 years ago by
This ticket's changes are plain wrong !
EDIT: can we easily undo this, quickly, please ??
comment:25 Changed 7 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 7 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 re-do the division clean-up in combinat. I am not happy about that.
comment:28 Changed 5 years ago by
Keywords: | divison added; python3 removed |
---|
New commits:
handling some more future divisions