Python 3 preparation: The semantic of map() function is changed

[wluebbe]

In Py2 map() returns a list, while in Py3 map() returns an iterator (as itertools.imap() does in Py2).

The tool 2to3 wraps map() usages with a call to list().
An alternative approach is to add from future_builtins import map and to check the code.

This ticket is tracked as a dependency of meta-ticket ticket:16052.

[wluebbe]

This first commit was generated by 2to3. It has failing doctests! These will be fixed with a separate commit.

---

New commits:

​8c8e5dc

trac #16073: changes generated by "2to3 -w -f map src/sage"

[git]

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

​b786139

trac #16073: fix an error exposed by the generated code

[wluebbe]

The problem was that in one module the standard library function list had been overwritten. Now all doctests pass.

[wluebbe]

The code would now certainly benefit from changing expressions like list(map(...)) into a list comprehension.

This is more pythonic, more readable (especially in the case list(map(lambda ...)) ) and it avoids redundant list creation in Python 2. But it's a lot of stuff to check and change ...

[wluebbe]

This patch will cause a lot of merge conflicts with #16067 filter. It seems advisable to wait until #16067 is merged and then regenerate this patch.

Somebody to review #16067 ... ;-)

[aapitzsch]

Rebased.

---

New commits:

​9ebf5fd

Merge remote-tracking branch 'origin/develop' into 16073

[git]

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

​dc803c1

remove some unnecessary conversions to list

[darij]

The patch is red.

Either way, shouldn't it be only wrapping some of the uses, rather than all of them?

[git]

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

​6ddc5fc

Merge remote-tracking branch 'origin/develop' into 16073

[darij]

OK, but as I expected there are some questionable things here.

- r = sum ( map( lambda x: x[2], cycle ) )
+ r = sum ( [x[2] for x in cycle] )

Wouldn't keeping the old line be better? Summing an iterator should be faster than summing a list which has been constructed merely to be summed.

[git]

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

​8331e84

remove more unnecessary conversions to list

[aapitzsch]

Replying to darij:

OK, but as I expected there are some questionable things here.

- r = sum ( map( lambda x: x[2], cycle ) )
+ r = sum ( [x[2] for x in cycle] )

Wouldn't keeping the old line be better? Summing an iterator should be faster than summing a list which has been constructed merely to be summed.

You are right.

In this case we should write

r = sum ( (x[2] for x in cycle) )

Feel free to fix it (and others), currently I have no time to check the necessity of list() wrapping.

[darij]

- return "".join(map(lambda x: self._character_to_code[x], string))
+ return "".join([self._character_to_code[x] for x in string])

Wondering if this is a good step forward, given that you can join an iterator to a string:

>>> map(lambda x: x+x, ['a','b','c'])
<map object at 0x7fd747ddee10>
>>> "blah".join(map(lambda x: x+x, ['a','b','c']))
'aablahbbblahcc'

(Python 3).

---

- map(
- lambda x: chr(ord(x) + 49),
- list(str(num[0]))),
+ [chr(ord(x) + 49) for x in list(str(num[0]))],

Can't we get rid of the list?

---

- a,b,c,d,e,f = map(G,R)
+ a,b,c,d,e,f = list(map(G,R))

This seems to not be necessary:

>>> a,b,c = iter([1,2,3])
>>> a
1
>>> b
2
>>> c
3

---

This is unnecessary:

- if all(map(lambda s: s.is_final, state.label())):
+ if all([s.is_final for s in state.label()]):

If anything then all definitely works on iterators.

---

Here is the thing that worries me the most: There are tools for automatic translation of python2 into 3. I am not sure if we should, or are going to, release them on the Sage library; but if we are, I would rather not do any manual work unless we can make sure that it will not interfere with the automated changes. I have a hunches that a well-written bot could do better than some of the changes in this patch (for instance, it should be a rule that all(map(...)) can stay as it is rather than being replaced by all(list(map(...)))), thus leading to better speed, whereas with the changes already done it would not simplify them back to what they should be. This does not speak against edits such as replacing list variables by list_ and likewise, which are the right thing to do either way. But I would be wary of making changes that possibly complicate the situation without knowing that it will become untangled when the time comes to actually switch to python 3.

[git]

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

​99e57e2

remove furthr unnecessary conversions to list

[aapitzsch]

Darij thanks. I hopefully made all the changes you suggested.

Replying to darij:

Here is the thing that worries me the most: There are tools for automatic translation of python2 into 3. I am not sure if we should, or are going to, release them on the Sage library; but if we are, I would rather not do any manual work unless we can make sure that it will not interfere with the automated changes. I have a hunches that a well-written bot could do better than some of the changes in this patch (for instance, it should be a rule that all(map(...)) can stay as it is rather than being replaced by all(list(map(...)))), thus leading to better speed, whereas with the changes already done it would not simplify them back to what they should be. This does not speak against edits such as replacing list variables by list_ and likewise, which are the right thing to do either way. But I would be wary of making changes that possibly complicate the situation without knowing that it will become untangled when the time comes to actually switch to python 3.

These tools don't work well in all cases and usually one can disable special fixes. So manual work won't interfere with automated changes.

The target of these changes is to simplify the switch to python 3 without complicating the situation. :)

[wluebbe]

I got

1 item had failures:
   3 of 211 in sage.tests.cmdline.test_executable
    [210 tests, 3 failures, 29.22 s]
----------------------------------------------------------------------
sage -t --long src/sage/tests/cmdline.py  # 3 doctests failed

while there were no failures with plain sage 6.4. I have not yet found the cause.

[aapitzsch]

Replying to wluebbe:

I got

1 item had failures:
   3 of 211 in sage.tests.cmdline.test_executable
    [210 tests, 3 failures, 29.22 s]
----------------------------------------------------------------------
sage -t --long src/sage/tests/cmdline.py  # 3 doctests failed

while there were no failures with plain sage 6.4. I have not yet found the cause.

I have no problems with cmdline.py. Results keep the same with and without merging origin/develop into 16073.

sage -t --long src/sage/tests/cmdline.py
    [210 tests, 32.88 s]
----------------------------------------------------------------------
All tests passed!

[wluebbe]

Strange! And Volker just released sage-6.5.beta0 and auto-merge failed again ... :-(

By the way, back in June I was working to replace map with list comprehension or generator expressions (mostly). It was not quite complete when I took some time off.

I want to give that approach a fresh start on sage-6.5.beta0. I will describe the reasoning tomorrow (or so ...).

[git]

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

​f47ac2b

Merge remote-tracking branch 'origin/develop' into 16073

[wluebbe]

I uploaded a new branch u/wluebbe/ticket/16073-2.

My goal is to replace all map calls with list comprehension or generator expressions. Currently there are still about 40 list(map(... in the code (somehow my regexp did not find them :-/ ) and a number of maps in the doctests. So there is still some work to do.

The drawback of wrapping map with list is that in Python2 this is redundant. If a context accepts iterators then in Python2 map cannot exploit this (as it returns a list). Remapping map in Python2 on its iterator version itertools.imap requires extra code.

The current branch does change list comprehensions to generator expressions in all possible cases. I would rather leave this refactoring to a new ticket since there are lots of applicable cases separate from 16073.

I am very much interested in feedback to u/wluebbe/ticket/16073-2.

[darij]

wluebbe: sorry for the stupid question, but what is the precise difference between the two branches? I cannot tell it from the post.

[wluebbe]

The 2 branches (the current public/16073 and u/wluebbe/ticket/16073-2) are alternative approaches. I did not want to be so impolite and just swap the branches :-) And I did not think it is appropriate to open a separate ticket.

[darij]

I have added your branch to #15379 (an accidental duplicate ticket I made a year ago) to be able to see it on the trac viewer.

I have to say I have no idea which of your two branches is the better one :/

[wluebbe]

My branch gets rid of (mostly) all map(...) and list(map(...)) calls replacing them with list comprehension or generator expressions. There are currently a few (about 40) list(map(...)) calls left over (as e.g. in src/sage/graphs/bipartite_graph.py lines 643ff).

[git]

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

​cf901f9

Merge remote-tracking branch 'origin/develop' into 16073

[chapoton]

does not apply

[git]

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

​2c1de15

Merge remote-tracking branch 'origin/develop' into 16073

[jdemeyer]

I find [func(_) for _ in L] more readable than list(map(func, L)).

[aapitzsch]

Replying to jdemeyer:

I find [func(_) for _ in L] more readable than list(map(func, L)).

I will change it.

[git]

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

​3ce7033

make 'list(map(..))' more readable

[git]

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

​02ceae7

revert changes in random_variable.py

[jdemeyer]

I'm sorry but this is much too boring to review.

[jdemeyer]

Are the changes scripted? If so, can you just tell me which script you used to generate them, then I'll review the script instead.

[aapitzsch]

Replying to jdemeyer:

Are the changes scripted? If so, can you just tell me which script you used to generate them, then I'll review the script instead.

There is no script. But if necessary we can split this ticket.

[git]

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

​ce34bea

Merge remote-tracking branch 'origin/develop' into 16073

[jdemeyer]

Replying to aapitzsch:

There is no script.

That actually worries me. If one has to manually review each individual change, that is a lot of changes to review and a lot of potential for mistakes also.

[aapitzsch]

I still can split this ticket. Would it be sufficient to move the changes in src/sage/combinat/ to a new ticket?

[jdemeyer]

Well, my preferred solution would be if the majority of the changes would be scripted.

Splitting would help also but maybe 2 pieces are not enough.

[git]

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

​2916b21

revert changes in src/sage/combinat

[git]

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

​038b947

revert more changes

[aapitzsch]

I added three additional tickets (#18472, #18473, #18474).

[darij]

Can this now be set to closed?

[wluebbe]

No, unfortunately there are still a significant number of maps in the code base.

@André: Can you please move the branch public/16073 from this ticket into a new, separate ticket. Then I will review the change. - Reason: I will create one (maybe two) extra tickets/changes (this is now #18531) for the leftover maps that I found during the review of #18473 and #18474. And I would prefer that this tickets only refers to the tickets containing the changes.

[git]

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

​2df6d84

Merge remote-tracking branch 'origin/develop' into 16073

[wluebbe]

There are still a lot of map() call in the code.

But not all occurrences must be removed: there are a number of places where a Py3 map()-iterator leads to the same result as the Py2 map().

Here is a long (but incomplete) list:

all(here_is_an_iterable_allowed)
any(here_is_an_iterable_allowed)
bytearray(here_is_an_iterable_allowed)
dict(here_is_an_iterable_of_key_value_pairs_allowed)
filter(function, here_is_an_iterable_allowed)
frozenset(here_is_an_iterable_allowed)
list(here_is_an_iterable_allowed)
map(function, here_is_an_iterable_allowed)
max(here_is_an_iterable_allowed)
min(here_is_an_iterable_allowed)
reduce(function, here_is_an_iterable_allowed)
set(here_is_an_iterable_allowed)
sorted(here_is_an_iterable_allowed)
sum(here_is_an_iterable_allowed)
tuple(here_is_an_iterable_allowed)
zip(here_is_an_iterable_allowed)

''.join(here_is_an_iterable_allowed)
a,b,c* = here_is_an_iterable_allowed
x[i:j] = here_is_an_iterable_allowed
a_dict.update(here_is_an_iterable_of_key_value_pairs_allowed)
file.writelines(here_is_an_iterable_allowed)
math.fsum(here_is_an_iterable_allowed)

and MANY functions in module itertools.

for i in here_is_an_iterable_allowed:
    pass
[2*x for x in here_is_an_iterable_allowed]
(2*x for x in here_is_an_iterable_allowed)

But if the use of map includes a lambda then replacing it with a list comprehension improves the readability!

[chapoton]

see #22121 and #22127 for steps in this direction

[chapoton]

another step in #23084

[chapoton]

another step in #23130

[chapoton]

next #24382

[chapoton]

next in #24572

[embray]

Some more in #25231

[embray]

Some more in #25418

[vklein]

Some more in #25925

[chapoton]

Probably one coud close this one too ? Agreed ?

[vklein]

Sure.