Replacing map(lambda()) calls with list compression

[mihir-w]

map(lambda(....)) is slower than list compression. Replaced about 40 such instances with list compression.

Partially Fixes #4940.

@asmeurer please review.

[toolforger]

Can we get rid of the zip call as well?
My knowledge of list comprehensions is more theoretical than from practical use, but I dimly recall that it's possible write something like x0 * x1 for x0, x1 in fct_list, MV.blades[2].

[mihir-w]

I had tried the above option along with few simple variations but kept on getting syntax errors. Hence just used zip. Does zip slower the code ? Or is to keep the code simple ?

[jcrist]

I dimly recall that it's possible write something like x0 * x1 for x0, x1 in fct_list, MV.blades[2].

This is not true, that doesn't work. What you can do though is index out into x0, x1. So the expression could read:

tuple(x0 * x1 for x0, x1 in zip(fct_lst, MV.blades[2]))

[toolforger]

Looks good to me. However, I'm not familiar enough with list comprehensions to be 100% sure that lambda expression and list comprehension are always equivalent, so somebody else will have to verify that.

I have made a few proposals for further improvement, but there's always the case of "do not fix more than one thing at a time in a PR, so it remains reviewable"; so maybe it's better to postpone these to a subsequent PR.

[jcrist]

extra space between [ and x.

[jcrist]

Looks good, besides my one last comment. Please fix, and squash your commits into one. Will merge after that.

[smichr]

A few lines were missed in boolalg:

1328:    return int(''.join(list(map(str, list(term)))), 2)
1355:    return list(map(int, s))

[mihir-w]

There are many instances of type list(map()) in the code. A grep shows nearly 175 such instances. Changing all of them at once seemed too big for a PR. Hence I only replaced map(lambda()) calls which are the slowest. Maybe this should be a new issue ? Or perhaps a part of #4940 or #9079 ?

[debugger22]

There are many instances of type list(map()) in the code. A grep shows nearly 175 such instances. Changing all of them at once seemed too big for a PR.

Keeping PR short is a good habit. But here you are just replacing one thing with other. I think it won't be a problem to review as test suite will take care of it.

[debugger22]

Hence I only replaced map(lambda()) calls which are the slowest.

I'm just curious to know. How did you filter?

[mihir-w]

What should I replace the list(map()) calls with? In python < 3, map() returns a list and hence adding an additional conversion to list does not make sense. However in python versions > 3, map() returns a map object and hence an explicit conversion to list is needed.

Or should I just go ahead and replace every map() instance with a list compression ? In issue #4940, asmeurer asked to test each instance before replacing. That would be a big task ( in terms of time) if each instance has to be checked.

I'm just curious to know. How did you filter?

Ran git grep "map(lambda".

[toolforger]

Actually I think doing PRs à la "do only one thing at a time" is a good idea, even in this case.
PRs with many and more-or-less-subtly different changes tend to get hard to review, simply because concentration will slack off quickly.
Also, I do not think we should rely on the tests to find all problems. They are a supplement to human attention, not a replacement.

[debugger22]

Or should I just go ahead and replace every map() instance with a list compression ?

This would be terrible as map() IS faster in some cases.

Perhaps you are right about the intensity of the task here at play.

[jcrist]

Calls to list(map(...)) can be changed later. We could add map to the compatibility file, so that python < 3 defines map = functools.imap. Then all calls to map that require a list will need a list(map(...)) around them.

This would be terrible as map() IS faster in some cases.

Can you give an example? It's always been my experience that map(builtin_function, data), and (builtin_function(i) for i in data) have had the same speed, and I'd imagine that map`s performance would be highest for builtin functions only.

I'm going to merge this now, as all map(lambda ...) have been replaced. A new PR can be opened for additional fixes.

[debugger22]

Here's an example from Aaron's comment on #4940.

In [1]: def f(x):
   ...:     return x**2
   ...: 

In [2]: a = range(100)

In [4]: %timeit map(f, a)
10000 loops, best of 3: 128 us per loop

In [5]: %timeit [f(x) for x in a]
10000 loops, best of 3: 162 us per loop

[smichr]

Partially Fixes #4940.

Since the "Fixes #4940" follows the auto-close syntax, the issue was closed when this was committed. If you don't want that to happen you should slip the adverb between the two as fixes (partially) #4940 or simply don't use the close/fix/resolve words at all and use something like partially addresses #4940. I'll let someone else open the issue again if they feel that is necessary.

[mihir-w]

Sorry for that. Will be careful next time.

[asmeurer]

I think the performance difference is negligible and a list comprehension is much more readable when the map would use a lambda.