Opened 5 years ago

Closed 5 years ago

#24382 closed enhancement (fixed)

py3: more care for map

Reported by: Frédéric Chapoton Owned by:
Priority: major Milestone: sage-8.2
Component: python3 Keywords:
Cc: Erik Bray, Jeroen Demeyer, Travis Scrimshaw, François Bissey Merged in:
Authors: Frédéric Chapoton Reviewers: Erik, Bray, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 7d9f9a0 (Commits, GitHub, GitLab) Commit: 7d9f9a0ac28c62494910f53205f939a4391fd606
Dependencies: Stopgaps:

Status badges

Description

as part of #16073

Change History (11)

comment:1 Changed 5 years ago by Frédéric Chapoton

Branch: u/chapoton/24382
Commit: 2017b14a164a9b31d9519ba8f48295bacec3554a
Status: newneeds_review

New commits:

f2affedwrap map with list in aspect_ratio
2017b14some more care for map in python3

comment:2 Changed 5 years ago by git

Commit: 2017b14a164a9b31d9519ba8f48295bacec3554abca4942bfcc86c0a3fe9857e9ac714c72e68dfe0

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

bca4942another fix for map

comment:3 Changed 5 years ago by Frédéric Chapoton

Cc: Erik Bray Jeroen Demeyer Travis Scrimshaw François Bissey added

green bot, please review

comment:4 Changed 5 years ago by Frédéric Chapoton

ping ?

comment:5 Changed 5 years ago by Erik Bray

Status: needs_reviewneeds_work

I fixed a number of these already in my own branch but differently--in most of these cases instead of list(map(...)) I think it makes just as much sense to rewrite them as a list comprehension.

For example instead of

self._aspect_ratio = list(map(float, v))

write

self._aspect_ratio = [float(x) for x in v]

Personally I find this more readable, but that's subjective. More importantly it's just as efficient if not moreso, and avoids the unnecessary list copying on Python 2.

comment:6 Changed 5 years ago by Frédéric Chapoton

Are you going to propose an alternative branch here ?

comment:7 Changed 5 years ago by git

Commit: bca4942bfcc86c0a3fe9857e9ac714c72e68dfe07d9f9a0ac28c62494910f53205f939a4391fd606

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

7d9f9a0using list comprehension instead of list(map())

comment:8 Changed 5 years ago by Frédéric Chapoton

Status: needs_workneeds_review

I have made a better branch, please review

comment:9 Changed 5 years ago by Frédéric Chapoton

green bot, please review

comment:10 Changed 5 years ago by Travis Scrimshaw

Reviewers: Erik, Bray, Travis Scrimshaw
Status: needs_reviewpositive_review

LGTM. Thanks.

comment:11 Changed 5 years ago by Volker Braun

Branch: u/chapoton/243827d9f9a0ac28c62494910f53205f939a4391fd606
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.