Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change all classic divisions to true divisions in combinat folder #20471

Closed
fchapoton opened this issue Apr 19, 2016 · 27 comments
Closed

Change all classic divisions to true divisions in combinat folder #20471

fchapoton opened this issue Apr 19, 2016 · 27 comments

Comments

@fchapoton
Copy link
Contributor

after #20468, let us take care of the rest of the combinat folder
so that all test pass when running

export PYTHONIOENCODING="utf-8"
python -Qnew $(which sage-runtests) src/sage/combinat/*.py 

in a sage shell.

Depends on #20468

Component: combinatorics

Keywords: combinat

Author: Frédéric Chapoton

Branch/Commit: cbeeb2c

Reviewer: Jeroen Demeyer, Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/20471

@fchapoton fchapoton added this to the sage-7.2 milestone Apr 19, 2016
@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/20471

@fchapoton
Copy link
Contributor Author

Commit: 4688e64

@fchapoton
Copy link
Contributor Author

New commits:

4688e64taking care of the remaining classic divisions in combinat folder

@jdemeyer
Copy link

comment:2

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 old-style divisions left in that module.

@fchapoton

This comment has been minimized.

@fchapoton
Copy link
Contributor Author

comment:3

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)

@jdemeyer
Copy link

comment:5

Replying to @fchapoton:

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 old-style 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.

@jdemeyer jdemeyer changed the title changing some classic division to exact division in combinat folder Change all classic divisions to true divisions in combinat folder Apr 20, 2016
@fchapoton
Copy link
Contributor Author

comment:7

What I mean is the following.

With my branch, running python -Qnew $(which sage-runtests) src/sage/combinat/*.py
gives no failing doctests.

Bur running python -Qwarnall $(which sage-runtests) 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 ?

@jdemeyer
Copy link

comment:8

Replying to @fchapoton:

Bur running python -Qwarnall $(which sage-runtests) 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.

@fchapoton
Copy link
Contributor Author

comment:9

Question: should I add the from future import in all of the combinat files, or only in the ones changed here or in #20468 ?

@jdemeyer
Copy link

Dependencies: #20468

@jdemeyer
Copy link

comment:10

I thought you checked all of them... in that case: all of them obviously.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 20, 2016

Changed commit from 4688e64 to fb736f3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 20, 2016

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

fb736f3trac 20471 adding from future import division in some combinat files

@fchapoton
Copy link
Contributor Author

comment:12

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 ?

@jdemeyer
Copy link

comment:13

Replying to @fchapoton:

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.

@fchapoton
Copy link
Contributor Author

comment:14

I would prefer not to make a patchbomb right here.

We maybe can discuss on sage-devel 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 ?

@jdemeyer
Copy link

comment:15

In alternating_sign_matrix, replace int(2) by 2.

@jdemeyer
Copy link

comment:16

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.

@jdemeyer
Copy link

comment:17

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 non-obvious change that I still need to check).

@jdemeyer
Copy link

Reviewer: Jeroen Demeyer

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 21, 2016

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

cbeeb2ctrac #20471 small details as suggested by reviewer

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 21, 2016

Changed commit from fb736f3 to cbeeb2c

@tscrim
Copy link
Collaborator

tscrim commented Apr 21, 2016

comment:20

The change in perfect_matching.py looks okay to me.

Is this ready for review again Frédéric?

@fchapoton
Copy link
Contributor Author

comment:21

yes, please

@jdemeyer
Copy link

Changed reviewer from Jeroen Demeyer to Jeroen Demeyer, Travis Scrimshaw

@vbraun
Copy link
Member

vbraun commented Apr 22, 2016

Changed branch from u/chapoton/20471 to cbeeb2c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants