classification
Title: Replace append loops with list comprehensions
Type: performance Stage: resolved
Components: Library (Lib) Versions: Python 3.8
process
Status: closed Resolution: not a bug
Dependencies: Superseder:
Assigned To: Nosy List: lgeiger, p-ganssle, remi.lapeyre, rhettinger, xtreak
Priority: normal Keywords:

Created on 2019-02-19 16:05 by lgeiger, last changed 2019-02-25 19:58 by lgeiger. This issue is now closed.

Messages (6)
msg335961 - (view) Author: Lukas Geiger (lgeiger) Date: 2019-02-19 16:05
Lib uses loops to append to a new list in quite a few places. I think it would be better to replace those with list comprehensions.

Benefits of this change:
  - List comprehensions are generally more readable than appending to a newly created list
  - List comprehensions are also a lot faster.
    Toy example:
    In [1]: %%timeit
       ...: l = []
       ...: for i in range(5000):
       ...:     l.append(i)
    375 µs ± 1.73 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

    In [2]: %%timeit
       ...: l = [i for i in range(5000)]
    168 µs ± 1.08 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)

Possible drawbacks:
  - Refactoring can always introduce bugs and makes it harder to get meaningful output from git blame. In this case I think the diff is very manageable, making the changes easy to review.

Personally, I think the codebase would benefit from this change both in terms of some small performance gains and maintainability. I'd be happy to make a PR to fix this.
msg335962 - (view) Author: Paul Ganssle (p-ganssle) * (Python committer) Date: 2019-02-19 16:11
I think I'm -1 on a general project or policy to replace all for-append loops with list comprehensions, but I'm generally positive about individual improvements to the code base, on a case-by-case basis.

I can't speak for anyone but myself, but I imagine that if you can identify a few places where this sort of change would make a significant performance improvement, it would be well-received.
msg335971 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2019-02-19 16:35
Agreed with @p-ganssle . Similar issue33152 in the past for the refactoring in timeit module to use list comprehension. Large refactorings like this could also make git blame little more difficult in places where this has less benefit like smaller lists for common cases. I think it's much better to identify hot code paths with potentially large lists that could benefit from this along with a benchmark that could be accepted by the core dev/maintainer of the module.
msg335973 - (view) Author: Rémi Lapeyre (remi.lapeyre) * Date: 2019-02-19 16:39
In some places, using a lot is also a deliberate choice to improve readability.

I think the boy scout rule is more appropriate for such changes.
msg336120 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-02-20 17:05
I concur with all the other -1 comments and will mark this a closed.

FWIW, we generally discourage sweeping across the library with minor rewrites.  Guido articulated a principle of "holistic refactoring" where we make code improvements while working on the module as a whole.  This helps makes sure that code changes are made in the context of a thorough understanding of what the module is trying to do.  This also helps us reduce maintenance-induced-bugs where minor code adjustments create new bugs that weren't there before.

Thank you for taking a look at the source and please continue to do so. If you find a specific case that is problematic, feel free to post that one particular case.  That said, a better use of time is to take one of the many open tracker issues, research it, and propose a fix.
msg336550 - (view) Author: Lukas Geiger (lgeiger) Date: 2019-02-25 19:58
Thank you very much for your thorough explanation, totally makes sense.
History
Date User Action Args
2019-02-25 19:58:17lgeigersetmessages: + msg336550
2019-02-20 17:05:51rhettingersetstatus: open -> closed

nosy: + rhettinger
messages: + msg336120

resolution: not a bug
stage: resolved
2019-02-19 16:39:28remi.lapeyresetnosy: + remi.lapeyre
messages: + msg335973
2019-02-19 16:35:05xtreaksetnosy: + xtreak
messages: + msg335971
2019-02-19 16:11:39p-gansslesetnosy: + p-ganssle
messages: + msg335962
2019-02-19 16:05:09lgeigercreate