violation of iterator protocol for Statevector

[Kwasniok]

Environment

• Qiskit Terra version: 0.20.1
• Python version: 3.10.4
• Operating system: Linux version 5.17.5

What is happening?

When iterating over a Statevector in a foor loop, data is attempted to be accessed after the last element.

How can we reproduce the issue?

import qiskit as qk
sv = qk.quantum_info.states.statevector.Statevector([1,2,3]) # same error for any amount of elements
for x in sv: 
    print(x)

Leads to

(1+0j)
(2+0j)
(3+0j)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../python3.10/site-packages/qiskit/quantum_info/states/statevector.py", line 214, in
 __getitem__
    raise QiskitError(f"Key {key} is greater than Statevector dimension {self.dim}.")
qiskit.exceptions.QiskitError: 'Key 3 is greater than Statevector dimension 3.'

What should happen?

EITHER:

• disallow iter resp .remove __iter__ (i.e. disable iteration)
• provide proper iterator which stops after last element see documentation

Adopting the latter would result in behavior like this:

import qiskit as qk
sv = qk.quantum_info.states.statevector.Statevector([1,2,3])
for x in sv: 
    print(x)

Leads to

(1+0j)
(2+0j)
(3+0j)

without any errors.

Any suggestions?

Since this iteration style is considered pythonic, it would be nice to have a proper iterator.
I suspect the iterator might be simply forwarded form the internal representation of the data.

Maybe also check other data types for this behavior?

[jakelishman]

Removing __iter__ actually doesn't disable iteration, which is where the bug really comes from; Statevector doesn't define __iter__, and wasn't originally intended to be used in iterable contexts. The problem is that Python "helpfully" tries to make any class that supports __getitem__ into an iterator by just trying successive integers in its __getitem__ until that method raises a KeyError, which Python catches and turns into StopIteration. Since Statevector raises QiskitError rather than KeyError (I'm not 100% sure why), the implicit form breaks.

The two possible solutions here are:

1. Provide a manual Statevector.__iter__ method that just iterates through the underlying _data attribute.
2. Change the errors into KeyError.

For our purposes, solution 1 is better, because we can do that without breaking backwards compatibility - it's possible (albeit unlikely) that some people are relying on Statevector throwing QiskitError on bad index access, so changing that would be a nuisance for them. Adding a proper __iter__ method is fine, though.

[Kwasniok]

Removing __iter__ actually doesn't disable iteration, which is where the bug really comes from...

Thank you @jakelishman! I did not know about this implicit rule. It explains the surprising behavior very well.

[ikkoham]

Personally, in relation to #8032, I'm against adding the explicit __iter__ method and I think we should explicitly use the data attribute. Including __getitem__, they can cause bugs and I don't like implicit conversions.

(@jakelishman Perhaps this is my preference, and we had a similar discussion at Fermion...)

[jakelishman]

Ikko: I completely agree with you if we didn't already have __getitem__ - I don't like this type of "implicit" reinterpretation of the data as something else, either. Since we've already got __getitem__, and we don't want to annoy people with removals, I think to fulfill the spirit properly we have to do __iter__ as well. It's not my preferred design style, but I'd rather have some code I don't much like than cause users problems for something which we don't have a technical need to remove.