Opened 3 years ago

Closed 3 years ago

#24382 closed enhancement (fixed)

py3: more care for map

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.2
Component: python3 Keywords:
Cc: embray, jdemeyer, tscrim, fbissey Merged in:
Authors: Frédéric Chapoton Reviewers: Erik, Bray, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 7d9f9a0 (Commits) Commit: 7d9f9a0ac28c62494910f53205f939a4391fd606
Dependencies: Stopgaps:

Description

as part of #16073

Change History (11)

comment:1 Changed 3 years ago by chapoton

  • Branch set to u/chapoton/24382
  • Commit set to 2017b14a164a9b31d9519ba8f48295bacec3554a
  • Status changed from new to needs_review

New commits:

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

comment:2 Changed 3 years ago by git

  • Commit changed from 2017b14a164a9b31d9519ba8f48295bacec3554a to bca4942bfcc86c0a3fe9857e9ac714c72e68dfe0

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

bca4942another fix for map

comment:3 Changed 3 years ago by chapoton

  • Cc embray jdemeyer tscrim fbissey added

green bot, please review

comment:4 Changed 3 years ago by chapoton

ping ?

comment:5 Changed 3 years ago by embray

  • Status changed from needs_review to needs_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 3 years ago by chapoton

Are you going to propose an alternative branch here ?

comment:7 Changed 3 years ago by git

  • Commit changed from bca4942bfcc86c0a3fe9857e9ac714c72e68dfe0 to 7d9f9a0ac28c62494910f53205f939a4391fd606

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

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

comment:8 Changed 3 years ago by chapoton

  • Status changed from needs_work to needs_review

I have made a better branch, please review

comment:9 Changed 3 years ago by chapoton

green bot, please review

comment:10 Changed 3 years ago by tscrim

  • Reviewers set to Erik, Bray, Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM. Thanks.

comment:11 Changed 3 years ago by vbraun

  • Branch changed from u/chapoton/24382 to 7d9f9a0ac28c62494910f53205f939a4391fd606
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.