Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add AerStatevector #1590

Merged
merged 76 commits into from
Sep 14, 2022
Merged

Add AerStatevector #1590

merged 76 commits into from
Sep 14, 2022

Conversation

hhorii
Copy link
Collaborator

@hhorii hhorii commented Sep 1, 2022

Summary

Add a new class AerStatevector that supports q.i.s.Statevector interface.

Details and comments

By using AerState (#1586), AerStatevector generates ndarray of a statevector.

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left quite a few comments (sorry) - since this is the actual user-facing interface, I had a bit more to say about structure and things like that.

I'm excited to see this in Aer, since it really starts making it easier to work with massive qubit systems from Python space. Do you happen to have any short benchmarks to hand about the performance of this compared to Terra's implementation?

A couple of top-level points:

  1. There's a few places where you call <ExplicitSuperClass>.<method> directly, or override stuff to do the same thing - just a reminder that all methods in Python are virtually dispatch, so self.<method> or super().<method> are usually what you want in those cases.
  2. There's a lot of faff around memory moving, and passing an underlying AerState instance around secretly to AerStatevector.__init__, which doesn't actually seem to accept that in its arguments. If you're constructing the Numpy arrays using the C API at some point, you might consider using PyArray_SetBaseObject to point to the owning AerState, then it'll always be accessible as data.base, and you won't need the "moved" status in AerState - AerState will always be responsible for freeing its underlying Numpy array (all ndarrays that view this one set their base to the same object, so the refcount of the AerState can't ever become 0 while there's active ndarray views).

qiskit_aer/quantum_info/states/aer_state.py Outdated Show resolved Hide resolved
Comment on lines 158 to 161
def __del__(self):
""""destructor. Call close() if not called."""
if self._state in (_STATE.ALLOCATED, _STATE.MOVED):
self.close()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__del__ in Python isn't guaranteed to be called at all, even when the Python interpreter exits. It usually gets called, but it's not for sure. A reliable way to ensure the finaliser always gets called is weakref.finalize - you'd call that immediately after allocating the memory in C++ space, I imagine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To call self.close() in a callback, weakref.finalize() needs to take self as an argument for the callback. However, taking self means self will never be collected. I guess that another way is necessary.

However, AerState is a private class. AerStatevector will not keep AerState as _STATE.ALLOCATED. I believe that __del__ is not necessary.

docs/apidocs/aer_quantum_info.rst Outdated Show resolved Hide resolved
qiskit_aer/quantum_info/states/__init__.py Outdated Show resolved Hide resolved
qiskit_aer/quantum_info/states/aer_statevector.py Outdated Show resolved Hide resolved
src/simulators/state_chunk.hpp Outdated Show resolved Hide resolved
test/terra/states/test_aer_statevector.py Outdated Show resolved Hide resolved
test/terra/states/test_aer_statevector.py Outdated Show resolved Hide resolved
test/terra/states/test_aer_statevector.py Show resolved Hide resolved
Comment on lines +45 to +46
class TestAerStatevector(common.QiskitAerTestCase):
"""AerState tests"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to provide some more stress tests of the memory allocation, moving and de-allocation routines, since there's quite a lot of complexity in there.

@hhorii
Copy link
Collaborator Author

hhorii commented Sep 9, 2022

@jakelishman

There's a lot of faff around memory moving, and passing an underlying AerState instance around secretly to AerStatevector.init, which doesn't actually seem to accept that in its arguments. If you're constructing the Numpy arrays using the C API at some point, you might consider using PyArray_SetBaseObject to point to the owning AerState, then it'll always be accessible as data.base, and you won't need the "moved" status in AerState - AerState will always be responsible for freeing its underlying Numpy array (all ndarrays that view this one set their base to the same object, so the refcount of the AerState can't ever become 0 while there's active ndarray views).

Returned ndarray can be huge and it should be freed as early as possible. I think its lifecycle may be shorter than owning AerState. Also, Aer has functions to free memory allocated in C++ as ndarray. I guess, though the current complex usages of move() will remain, memory should be freed when its ndarray is collected.

@hhorii hhorii mentioned this pull request Sep 13, 2022
4 tasks
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor comments, but nothing that hugely needs to hold anything up.

def __copy__(self):
ret = AerStatevector(self._data.copy(), **self._configs)
ret._op_shape = self._op_shape
ret._rng_generator = self._rng_generator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line looks dangerous - it'll cause two objects to hold references to the same rng instance, so they'll both be advancing state of the same thing. I don't know what's most appropriate here, but I'd expect maybe a copy of this?

Comment on lines 109 to 111
def __eq__(self, other):
return (isinstance(other, AerStatevector) and self.dims() == other.dims() and
np.allclose(self._data, other._data, rtol=self.rtol, atol=self.atol))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The isinstance check here is likely too strict - it'll cause equality not to be commutative when comparing with a raw Statevector instance. Statevector.__eq__(AerStatevector) (for the same data) will be true (because issubclass(AerStatevector, Statevector), but AerStatevector.__eq__(Statevector) will be false.

That said, the fuzzy np.allclose already breaks equality transitivity (a == b and b == c doesn't imply a == c), but that's the fault of the base class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that custom __eq__ does not need. The updated version uses __eq__ of the super class.

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last change squashed the last couple of major issues I had seen, thanks. I'm not ideally placed to evaluate whether the interface satisfies exactly what you need, and I don't want to hold this up any further. Let's move to merge and get Aer 0.11 out soon.

@hhorii hhorii merged commit 3babb7f into Qiskit:main Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants