0

I have a list of four numbers:

mylist=[3,5,67,4]

I want to remove all the odd numbers. So, I've written the following:

for item in mylist:
  if item%2==1:
    mylist.remove(item)

When I print mylist, I get the following:

[5,4]

I cannot figure out why this is happening. However, when I add a print statement after the if statement I get the correct answer:

for item in mylist:
  if item%2==1:
    mylist.remove(item)
  print mylist

which yields:

[4]

What's going on here? What am I missing?

Fezter
  • 221
  • 1
  • 3
  • 12
  • This is a common mistake that happens when you mutate a sequence while iterating over it. – mgilson Jun 04 '13 at 00:06
  • Of course. I don't know why I didn't see it. I get it now. Also, I agree that this is a duplicate question and have flagged it as such. – Fezter Jun 04 '13 at 01:10

3 Answers3

5

It's a bad idea to modify a list in-place while iterating it. As the list changes while you iterate over it, your iteration continues as though the list were unaltered - producing weird results.

The best solution is to build a new list - optimally with a list comprehension instead:

mylist = [item for item in mylist if item % 2 == 0]

If you have to modify the list, you can assign the values back afterwards (mylist[:] = ...), but it's unlikely you need to modify it in-place.

This also has the advantage of being readable and concise.

Gareth Latty
  • 86,389
  • 17
  • 178
  • 183
  • a more robust solution will be `mylist[:] = mylist[::2]`. Otherwise, in a different situation, he may add a local binding accidentaly. – Elazar Jun 04 '13 at 00:13
  • 1
    @Elazar: That doesn't solve the original problem, which is to remove all odd numbers. That simply returns a list containing every other item from the original list. – Justin S Barrett Jun 04 '13 at 00:14
  • @Elazar That doesn't solve the problem, and it's not necessary in most cases to actually modify the existing list, as I stated in the answer. – Gareth Latty Jun 04 '13 at 00:14
  • @Elazar I've clarified that as an option, but as I pointed out, it's highly unlikely that it needs to be done in-place, and assigning it back is a less efficient operation, so it should only be done if it needs to be done. – Gareth Latty Jun 04 '13 at 00:15
  • I'm fine with your revised answer. But I think efficiency is a concern only if it explicitly so. – Elazar Jun 04 '13 at 00:18
  • @Elazar Premature optimisation isn't worthwhile - but here we are talking about a case where it's easier to do it the more optimised way. Modifying the list is more likely to be a downside than it is necessary. In most cases, working on the new list is the right route. In fact, code that relies on the list being modified in-place is probably fragile and could probably use changing. If someone needs to do it, they'll know they need to. – Gareth Latty Jun 04 '13 at 00:20
  • OK. Fair enough. But you make a new list, it should have a new name. – Elazar Jun 04 '13 at 00:23
  • @Elazar If you have no use for the old list any more, there is no reason not to reuse the old name and let the old one be garbage collected. – Gareth Latty Jun 04 '13 at 00:24
  • There is. you have a global list `mylist`, you do the assignment, all of a sudden you are referring to a *local* list instead. – Elazar Jun 04 '13 at 00:25
  • That's a very specific case, yes, in that case it might be a better idea to use another name. That said, globals are *generally* a bad idea anyway. I don't really see your point with this - yes, there are some cases where you would want to assign back or use a different name, but this is a general answer, I can't hope to give it for every possible scenario. – Gareth Latty Jun 04 '13 at 00:28
  • you can: use an appropriate name. new names for new lists, old names for old lists. changing in-place if that's what needed, while bearing in mind it probably isn't. That assignment statement may not be what you meant it to be (and not only with global variables), so it should be avoided if possible. – Elazar Jun 04 '13 at 00:34
  • I disagree - there is nothing inherently wrong with reusing a name. It depends on the situation. Yes, naturally you should modify the answer to suit your needs, but that's a given with any answer on SO. – Gareth Latty Jun 04 '13 at 00:51
  • Thanks for this answer. +1 for linking to your great video. – Fezter Jun 04 '13 at 01:23
3

You need to iterate over a copy of the list. Modifying the list directly in a loop is what's causing the problem you experienced. Here's the preferred way to do it:

for item in mylist[:]:
    if item%2==1:
        mylist.remove(item)
Justin S Barrett
  • 636
  • 4
  • 14
0

I agree with the other answers that say that it's not safe to iterate over a list and modify that list in the loop. One solution I have found for such problems - specifically removing from a list - is to loop backwards using an index rather than an iterator.

My python code is rusty so I'll use C-like pseudocode instead...

for ( i = lastindex; i >= 0; --i )
  if ( some condition involving list[i] )
    remove item at index i

This works when removing from the list because the items shifted by the removal are the ones you've already looked at; your i index is still valid as are all the items you haven't evaluated yet!

aldo
  • 2,927
  • 21
  • 36
  • Looping by index isn't Pythonic - it's inflexible (doesn't work on iterators, just sequences), it's harder to read, and it's slow. – Gareth Latty Jun 04 '13 at 11:43