Unexpected behaviour with a conditional generator expression [duplicate]
Asked Answered
E

8

58

I was running a piece of code that unexpectedly gave a logic error at one part of the program. When investigating the section, I created a test file to test the set of statements being run and found out an unusual bug that seems very odd.

I tested this simple code:

array = [1, 2, 2, 4, 5] # Original array
f = (x for x in array if array.count(x) == 2) # Filters original
array = [5, 6, 1, 2, 9] # Updates original to something else

print(list(f)) # Outputs filtered

And the output was:

>>> []

Yes, nothing. I was expecting the filter comprehension to get items in the array with a count of 2 and output this, but I didn't get that:

# Expected output
>>> [2, 2]

When I commented out the third line to test it once again:

array = [1, 2, 2, 4, 5] # Original array
f = (x for x in array if array.count(x) == 2) # Filters original
### array = [5, 6, 1, 2, 9] # Ignore line

print(list(f)) # Outputs filtered

The output was correct (you can test it for yourself):

>>> [2, 2]

At one point I outputted the type of the variable f:

array = [1, 2, 2, 4, 5] # Original array
f = (x for x in array if array.count(x) == 2) # Filters original
array = [5, 6, 1, 2, 9] # Updates original

print(type(f))
print(list(f)) # Outputs filtered

And I got:

>>> <class 'generator'>
>>> []

Why is updating a list in Python changing the output of another generator variable? This seems very odd to me.

Excreta answered 17/1, 2019 at 23:11 Comment(3)
You redefine array and your new array is what gets referenced by the lazy generator comprehension.Differ
Would be good to see an answer that mentions scope.Stanchion
This is a variation of the question of "late binding" of python closures. The generator is essentially acting like a closure here. (I'm not sure why the answers are so focused on laziness... that, I think, is obvious to anyone using a generator.)Gloxinia
M
64

Python's generator expressions are late binding (see PEP 289 -- Generator Expressions) (what the other answers call "lazy"):

Early Binding versus Late Binding

After much discussion, it was decided that the first (outermost) for-expression [of the generator expression] should be evaluated immediately and that the remaining expressions be evaluated when the generator is executed.

[...] Python takes a late binding approach to lambda expressions and has no precedent for automatic, early binding. It was felt that introducing a new paradigm would unnecessarily introduce complexity.

After exploring many possibilities, a consensus emerged that binding issues were hard to understand and that users should be strongly encouraged to use generator expressions inside functions that consume their arguments immediately. For more complex applications, full generator definitions are always superior in terms of being obvious about scope, lifetime, and binding.

That means it only evaluates the outermost for when creating the generator expression. So it actually binds the value with the name array in the "subexpression" in array (in fact it's binding the equivalent to iter(array) at this point). But when you iterate over the generator the if array.count call actually refers to what is currently named array.


Since it's actually a list not an array I changed the variable names in the rest of the answer to be more accurate.

In your first case the list you iterate over and the list you count in will be different. It's as if you used:

list1 = [1, 2, 2, 4, 5]
list2 = [5, 6, 1, 2, 9]
f = (x for x in list1 if list2.count(x) == 2)

So you check for each element in list1 if its count in list2 is two.

You can easily verify this by modifying the second list:

>>> lst = [1, 2, 2]
>>> f = (x for x in lst if lst.count(x) == 2)
>>> lst = [1, 1, 2]
>>> list(f)
[1]

If it iterated over the first list and counted in the first list it would've returned [2, 2] (because the first list contains two 2). If it iterated over and counted in the second list the output should be [1, 1]. But since it iterates over the first list (containing one 1) but checks the second list (which contains two 1s) the output is just a single 1.

Solution using a generator function

There are several possible solutions, I generally prefer not to use "generator expressions" if they aren't iterated over immediately. A simple generator function will suffice to make it work correctly:

def keep_only_duplicated_items(lst):
    for item in lst:
        if lst.count(item) == 2:
            yield item

And then use it like this:

lst = [1, 2, 2, 4, 5]
f = keep_only_duplicated_items(lst)
lst = [5, 6, 1, 2, 9]

>>> list(f)
[2, 2]

Note that the PEP (see the link above) also states that for anything more complicated a full generator definition is preferrable.

A better solution using a generator function with a Counter

A better solution (avoiding the quadratic runtime behavior because you iterate over the whole array for each element in the array) would be to count (collections.Counter) the elements once and then do the lookup in constant time (resulting in linear time):

from collections import Counter

def keep_only_duplicated_items(lst):
    cnts = Counter(lst)
    for item in lst:
        if cnts[item] == 2:
            yield item

Appendix: Using a subclass to "visualize" what happens and when it happens

It's quite easy to create a list subclass that prints when specific methods are called, so one can verify that it really works like that.

In this case I just override the methods __iter__ and count because I'm interested over which list the generator expression iterates and in which list it counts. The method bodies actually just delegate to the superclass and print something (since it uses super without arguments and f-strings it requires Python 3.6 but it should be easy to adapt for other Python versions):

class MyList(list):
    def __iter__(self):
        print(f'__iter__() called on {self!r}')
        return super().__iter__()
        
    def count(self, item):
        cnt = super().count(item)
        print(f'count({item!r}) called on {self!r}, result: {cnt}')
        return cnt

This is a simple subclass just printing when the __iter__ and count method are called:

>>> lst = MyList([1, 2, 2, 4, 5])

>>> f = (x for x in lst if lst.count(x) == 2)
__iter__() called on [1, 2, 2, 4, 5]

>>> lst = MyList([5, 6, 1, 2, 9])

>>> print(list(f))
count(1) called on [5, 6, 1, 2, 9], result: 1
count(2) called on [5, 6, 1, 2, 9], result: 1
count(2) called on [5, 6, 1, 2, 9], result: 1
count(4) called on [5, 6, 1, 2, 9], result: 0
count(5) called on [5, 6, 1, 2, 9], result: 1
[]
Mono answered 18/1, 2019 at 7:43 Comment(10)
This is the only answer that explains all the subtleties involved in the questioned behavior.Sporophyll
Your example as given (with result [1]) might only look at the second list. It would be even better if you used something like [1, 1, 2, 2, 3, 4, 5] and [1, 2, 2, 3, 3, 4, 6], with result [2, 2, 3].Sporophyll
See for example tio.run/…Sporophyll
@Sporophyll Thank you for the additional example. But I'm not sure what you mean with my example being ambguous. I thought in case it would look only at the first list the result would be [2,2], if it would only look at the second list the result would be [1, 1]. That the result is [1] shows that it iterates over the first list, but filters based on the second list. Is my thinking incorrect there?Mono
MSeifert: You are entirely correct. In my head I was running the program without considering duplicates, which lead me astray. Still I believe there may be some additional clarity to be won in the direction I indicated, for those similarly afflicted as I am.Sporophyll
Wow, that's about as counter-intuitive as it gets. Usually Python is easier to explain than that.Neuman
Just a tiny correction: the generator expression never really binds array. It binds the equivalent of iter(array) (the difference is that __iter__ is called when creating the generator expresision, not when you start consuming it).Wildfowl
You can use def keep_only_duplicated_items(lst): yield from (x for x in lst if lst.count(x) == 2) if you wish to keep the generator syntax, but also capture the original lst reference.Gloxinia
@MateenUlhaq Yeah, there's another answer using that approach. However in my opinion it's not that readable so I would still prefer the custom generator.Mono
@GiacomoAlzetta You're right. I changed the answer to clarify that point. Thank youMono
F
18

As others have mentioned Python generators are lazy. When this line is run:

f = (x for x in array if array.count(x) == 2) # Filters original

nothing actually happens yet. You've just declared how the generator function f will work. Array is not looked at yet. Then, you create a new array that replaces the first one, and finally when you call

print(list(f)) # Outputs filtered

the generator now needs the actual values and starts pulling them from the generator f. But at this point, array already refers to the second one, so you get an empty list.

If you need to reassign the list, and can't use a different variable to hold it, consider creating the list instead of a generator in the second line:

f = [x for x in array if array.count(x) == 2] # Filters original
...
print(f)
Fanchette answered 17/1, 2019 at 23:22 Comment(1)
This is incorrect. As https://mcmap.net/q/329786/-unexpected-behaviour-with-a-conditional-generator-expression-duplicate explains array in in array is bound immediately but array in array.count only later. You could also try to explain tio.run/…Sporophyll
G
11

Others have already explained the root cause of the issue - the generator is binding to the name of the array local variable, rather than its value.

The most pythonic solution is definitely the list comprehension:

f = [x for x in array if array.count(x) == 2]

However, if there is some reason that you don't want to create a list, you can also force a scope close over array:

f = (lambda array=array: (x for x in array if array.count(x) == 2))()

What's happening here is that the lambda captures the reference to array at the time the line is run, ensuring that the generator sees the variable you expect, even if the variable is later redefined.

Note that this still binds to the variable (reference), not the value, so, for example, the following will print [2, 2, 4, 4]:

array = [1, 2, 2, 4, 5] # Original array

f = (lambda array=array: (x for x in array if array.count(x) == 2))() # Close over array
array.append(4)  # This *will* be captured

array = [5, 6, 1, 2, 9] # Updates original to something else

print(list(f)) # Outputs [2, 2, 4, 4]

This is a common pattern in some languages, but it's not very pythonic, so only really makes sense if there's a very good reason for not using the list comprehension (e.g., if array is very long, or is being used in a nested generator comprehension, and you're concerned about memory).

Gassy answered 18/1, 2019 at 7:11 Comment(1)
Useful answer for showing how to override the default behavior!Sporophyll
Y
7

You are not using a generator correctly if this is the primary use of this code. Use a list comprehension instead of a generator comprehension. Just replace the parentheses with brackets. It evaluates to a list if you don't know.

array = [1, 2, 2, 4, 5]
f = [x for x in array if array.count(x) == 2]
array = [5, 6, 1, 2, 9]

print(f)
#[2, 2]

You are getting this response because of the nature of a generator. You're calling the generator when it't contents will evaluate to []

Yulandayule answered 17/1, 2019 at 23:19 Comment(5)
Thank you. I seem to have used the wrong brackets. But in general using a generator comprehension seems odd.Excreta
With your change, list(f) becomes redundant.Neuman
Lol @Mark Ransom, copy paste got me, I edited.Yulandayule
@SurajKothari It is not odd, it's a great tool! It just takes some time to wrap the ole brain around. Do some research you'll find that generators are amazing!Yulandayule
This does not explain the observed behavior and so does not answer the question.Sporophyll
N
5

Generators are lazy, they won't be evaluated until you iterate through them. In this case that's at the point you create the list with the generator as input, at the print.

Neuman answered 17/1, 2019 at 23:15 Comment(6)
When am I iterating through them. Am I meant to?Excreta
@SurajKothari when you create the list it will iterate for you without you needing to do it explicitly.Neuman
Also which list? When I declare the first one, or re-assign the second?Excreta
What first & second? You define only one list, at the final line of your code.Hyperbola
@SurajKothari I updated the answer to be more clear.Neuman
This could have been my own answer, but it is incorrect (see MSeifert's answer) or try to explain tio.run/…Sporophyll
C
4

The root cause of the problem is that generators are lazy; variables are evaluated each time:

>>> l = [1, 2, 2, 4, 5, 5, 5]
>>> filtered = (x for x in l if l.count(x) == 2)
>>> l = [1, 2, 4, 4, 5, 6, 6]
>>> list(filtered)
[4]

It iterates over the original list and evaluates the condition with the current list. In this case, 4 appeared twice in the new list, causing it to appear in the result. It only appears once in the result because it only appeared once in the original list. The 6s appear twice in the new list, but never appear in the old list and are hence never shown.

Full function introspection for the curious (the line with the comment is the important line):

>>> l = [1, 2, 2, 4, 5]
>>> filtered = (x for x in l if l.count(x) == 2)
>>> l = [1, 2, 4, 4, 5, 6, 6]
>>> list(filtered)
[4]
>>> def f(original, new, count):
    current = original
    filtered = (x for x in current if current.count(x) == count)
    current = new
    return list(filtered)

>>> from dis import dis
>>> dis(f)
  2           0 LOAD_FAST                0 (original)
              3 STORE_DEREF              1 (current)

  3           6 LOAD_CLOSURE             0 (count)
              9 LOAD_CLOSURE             1 (current)
             12 BUILD_TUPLE              2
             15 LOAD_CONST               1 (<code object <genexpr> at 0x02DD36B0, file "<pyshell#17>", line 3>)
             18 LOAD_CONST               2 ('f.<locals>.<genexpr>')
             21 MAKE_CLOSURE             0
             24 LOAD_DEREF               1 (current)
             27 GET_ITER
             28 CALL_FUNCTION            1 (1 positional, 0 keyword pair)
             31 STORE_FAST               3 (filtered)

  4          34 LOAD_FAST                1 (new)
             37 STORE_DEREF              1 (current)

  5          40 LOAD_GLOBAL              0 (list)
             43 LOAD_FAST                3 (filtered)
             46 CALL_FUNCTION            1 (1 positional, 0 keyword pair)
             49 RETURN_VALUE
>>> f.__code__.co_varnames
('original', 'new', 'count', 'filtered')
>>> f.__code__.co_cellvars
('count', 'current')
>>> f.__code__.co_consts
(None, <code object <genexpr> at 0x02DD36B0, file "<pyshell#17>", line 3>, 'f.<locals>.<genexpr>')
>>> f.__code__.co_consts[1]
<code object <genexpr> at 0x02DD36B0, file "<pyshell#17>", line 3>
>>> dis(f.__code__.co_consts[1])
  3           0 LOAD_FAST                0 (.0)
        >>    3 FOR_ITER                32 (to 38)
              6 STORE_FAST               1 (x)
              9 LOAD_DEREF               1 (current)  # This loads the current list every time, as opposed to loading a constant.
             12 LOAD_ATTR                0 (count)
             15 LOAD_FAST                1 (x)
             18 CALL_FUNCTION            1 (1 positional, 0 keyword pair)
             21 LOAD_DEREF               0 (count)
             24 COMPARE_OP               2 (==)
             27 POP_JUMP_IF_FALSE        3
             30 LOAD_FAST                1 (x)
             33 YIELD_VALUE
             34 POP_TOP
             35 JUMP_ABSOLUTE            3
        >>   38 LOAD_CONST               0 (None)
             41 RETURN_VALUE
>>> f.__code__.co_consts[1].co_consts
(None,)

To reiterate: The list to be iterated is only loaded once. Any closures in the condition or expression, however, are loaded from the enclosing scope each iteration. They are not stored in a constant.

The best solution for your problem would be to create a new variable referencing the original list and use that in your generator expression,.

Chamfron answered 18/1, 2019 at 3:25 Comment(0)
H
2

Generator evaluation is "lazy" -- it doesn't get executed until you actualize it with a proper reference. With your line:

Look again at your output with the type of f: that object is a generator, not a sequence. It's waiting to be used, an iterator of sorts.

Your generator isn't evaluated until you start requiring values from it. At that point, it uses the available values at that point, not the point at which it was defined.


Code to "make it work"

That depends on what you mean by "make it work". If you want f to be a filtered list, then use a list, not a generator:

f = [x for x in array if array.count(x) == 2] # Filters original
Hyperbola answered 17/1, 2019 at 23:17 Comment(1)
I somewhat understand. Could you show some code to make it work, because I need to re-assign the same list again in the main code.Excreta
D
2

Generators are lazy and your newly defined array is used when you exhaust your generator after redefining. Therefore, the output is correct. A quick fix is to use a list comprehension by replacing parentheses () by brackets [].

Moving on to how better to write your logic, counting a value in a loop has quadratic complexity. For an algorithm that works in linear time, you can use collections.Counter to count values, and keep a copy of your original list:

from collections import Counter

array = [1, 2, 2, 4, 5]   # original array
counts = Counter(array)   # count each value in array
old_array = array.copy()  # make copy
array = [5, 6, 1, 2, 9]   # updates array

# order relevant
res = [x for x in old_array if counts[x] >= 2]
print(res)
# [2, 2]

# order irrelevant
from itertools import chain
res = list(chain.from_iterable([x]*count for x, count in counts.items() if count >= 2))
print(res)
# [2, 2]

Notice the second version doesn't even require old_array and is useful if there is no need to maintain ordering of values in your original array.

Differ answered 18/1, 2019 at 1:32 Comment(0)

© 2022 - 2024 — McMap. All rights reserved.