Opened 5 years ago
Closed 5 years ago
#20471 closed enhancement (fixed)
Change all classic divisions to true divisions in combinat folder
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage7.2 
Component:  combinatorics  Keywords:  python3, combinat 
Cc:  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Jeroen Demeyer, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  cbeeb2c (Commits)  Commit:  cbeeb2c6514f9b3dcf2d27f04e2e675e93e931fd 
Dependencies:  #20468  Stopgaps: 
Description (last modified by )
after #20468, let us take care of the rest of the combinat folder so that all test pass when running
export PYTHONIOENCODING="utf8" python Qnew $(which sageruntests) src/sage/combinat/*.py
in a sage shell.
Change History (23)
comment:1 Changed 5 years ago by
 Branch set to u/chapoton/20471
 Commit set to 4688e6411e5506ccbcdd70d690658c4d3f26a522
 Status changed from new to needs_review
comment:2 Changed 5 years ago by
General suggestion: add from __future__ import division
on top of modules which have been fully converted. That way we know for sure that there are no oldstyle divisions left in that module.
comment:3 followup: ↓ 5 Changed 5 years ago by
 Description modified (diff)
well, I understand, but I only took care of the failing doctests, not of the warnings (that often come from another part of sage, in fact)
comment:4 Changed 5 years ago by
 Status changed from needs_review to needs_info
comment:5 in reply to: ↑ 3 Changed 5 years ago by
Replying to chapoton:
I only took care of the failing doctests, not of the warnings (that often come from another part of sage, in fact)
I really do not understand what you mean. In fact, it makes me doubt that you understood what I meant:
if you have removed all classic divisions from the combinat
folder, I recommend to put from __future__ import division
in the source code of the sage/combinat/*.py(x)
files. Doing that will ensure that no oldstyle division can sneak in with future changes. It also makes it very explicit that no further work needs to be done with division in those modules.
comment:6 Changed 5 years ago by
 Summary changed from changing some classic division to exact division in combinat folder to Change all classic divisions to true divisions in combinat folder
comment:7 followup: ↓ 8 Changed 5 years ago by
What I mean is the following.
With my branch, running python Qnew $(which sageruntests) src/sage/combinat/*.py
gives no failing doctests.
Bur running python Qwarnall $(which sageruntests) src/sage/combinat/*.py
still gives many warnings. Most of these warnings are related to classic division
happening in other parts of sage, but there could be some still happening in /combinat
without affecting the doctests.
I could try to add the from future import
everywhere in all \combinat files and
see if no problem occurs. Would this be a good idea ?
comment:8 in reply to: ↑ 7 Changed 5 years ago by
Replying to chapoton:
Bur running
python Qwarnall $(which sageruntests) src/sage/combinat/*.py
still gives many warnings. Most of these warnings are related to classic division happening in other parts of sage
That does not matter.
but there could be some still happening in /combinat without affecting the doctests.
That could only happen if there are divisions which are not executed by any doctest. Let's hope this is not the case... in any case, it is hard to check otherwise.
I could try to add the
from future import
everywhere in all \combinat files and see if no problem occurs. Would this be a good idea ?
Yes, that is what I meant.
comment:9 Changed 5 years ago by
Question: should I add the from future
import in all of the combinat files, or only in the ones changed here or in #20468 ?
comment:10 Changed 5 years ago by
 Dependencies set to #20468
I thought you checked all of them... in that case: all of them obviously.
comment:11 Changed 5 years ago by
 Commit changed from 4688e6411e5506ccbcdd70d690658c4d3f26a522 to fb736f3818af93f9849949f2c7eb97080c4fce27
Branch pushed to git repo; I updated commit sha1. New commits:
fb736f3  trac 20471 adding from future import division in some combinat files

comment:12 followup: ↓ 13 Changed 5 years ago by
But many of them do not use division at all !
Wouldn't it be enough to just mark those where I did change something in the divisions ?
Or are we planning to have from future import division
in every single python file in sage ?
comment:13 in reply to: ↑ 12 Changed 5 years ago by
Replying to chapoton:
Or are we planning to have
from future import division
in every single python file in sage ?
This hasn't really been formally discussed. I would be in favour of having it in every module.
comment:14 Changed 5 years ago by
I would prefer not to make a patchbomb right here.
We maybe can discuss on sagedevel the general question of adding from future import division
everywhere.
Would you still agree to give a positive review for this ticket in its current state ?
comment:15 Changed 5 years ago by
In alternating_sign_matrix
, replace int(2)
by 2
.
comment:16 Changed 5 years ago by
You changed this to really weird spacing: range(1,len(tst) // 2+1)
Usually, I don't mind spacing but I just noticed because you actually changed it.
comment:17 Changed 5 years ago by
All fine except for the two comments above and the change in src/sage/combinat/perfect_matching.py
(not because it is wrong, but because it is a nonobvious change that I still need to check).
comment:18 Changed 5 years ago by
 Reviewers set to Jeroen Demeyer
 Status changed from needs_info to needs_work
comment:19 Changed 5 years ago by
 Commit changed from fb736f3818af93f9849949f2c7eb97080c4fce27 to cbeeb2c6514f9b3dcf2d27f04e2e675e93e931fd
Branch pushed to git repo; I updated commit sha1. New commits:
cbeeb2c  trac #20471 small details as suggested by reviewer

comment:20 Changed 5 years ago by
The change in perfect_matching.py
looks okay to me.
Is this ready for review again Frédéric?
comment:22 Changed 5 years ago by
 Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Travis Scrimshaw
 Status changed from needs_review to positive_review
comment:23 Changed 5 years ago by
 Branch changed from u/chapoton/20471 to cbeeb2c6514f9b3dcf2d27f04e2e675e93e931fd
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
taking care of the remaining classic divisions in combinat folder