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

get rid of part of itervalues #29103

Closed
fchapoton opened this issue Jan 29, 2020 · 14 comments
Closed

get rid of part of itervalues #29103

fchapoton opened this issue Jan 29, 2020 · 14 comments

Comments

@fchapoton
Copy link
Contributor

after #29077

There are much more cases, so only half of them is taken care of.

CC: @tscrim @jm58660

Component: refactoring

Author: Frédéric Chapoton

Branch/Commit: cbb20e6

Reviewer: Travis Scrimshaw

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

@fchapoton fchapoton added this to the sage-9.1 milestone Jan 29, 2020
@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/29103

@fchapoton
Copy link
Contributor Author

New commits:

47f6d46first parameter must be self
7072bceget rid of some calls to six.itervalues

@fchapoton
Copy link
Contributor Author

Commit: 7072bce

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 29, 2020

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7cbd2c2get rid of some call to six.itervalues

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 29, 2020

Changed commit from 7072bce to 7cbd2c2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 29, 2020

Changed commit from 7cbd2c2 to cbb20e6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 29, 2020

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

cbb20e6fix pyflakes warning

@fchapoton
Copy link
Contributor Author

comment:4

green bot, please review

@tscrim
Copy link
Collaborator

tscrim commented Jan 30, 2020

comment:5

I am not sure we should be doing this yet given that 9.1 will still be supporting Python2. Although I believe from the sage-devel discussion, are considering 9.0 the last properly Python2 supported version of Sage. So by that, it would be good to do it now. Although if 9.1 is the last such one, then we should wait a little bit I think.

@fchapoton
Copy link
Contributor Author

comment:6

I did that carefully in such a way that this remains fully python2-compatible.

@tscrim
Copy link
Collaborator

tscrim commented Jan 30, 2020

comment:7

Sorry, my response was poorly phrased. I meant the efficiency aspects with entire lists being created and returned in Python2 with these changes. I agree that it is fully Python2-compatible, but there will be a speed/memory difference between the two. Admittedly, this is not a very strong position considering the cases where this would matter are somewhat rare IMO. If you think that we should go ahead with only caring about Python3 at this point in time, then we can set this to a positive review.

@tscrim
Copy link
Collaborator

tscrim commented Jan 30, 2020

Reviewer: Travis Scrimshaw

@fchapoton
Copy link
Contributor Author

comment:8

I think that people cannot both ask for speed with python2 and use the latest sage..

Therefore setting to positive

@vbraun
Copy link
Member

vbraun commented Feb 10, 2020

Changed branch from u/chapoton/29103 to cbb20e6

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

3 participants